From c53e991e7b3add7f734f47cce1e7c348a92f6f23 Mon Sep 17 00:00:00 2001 From: Brian Harrington Date: Mon, 9 Oct 2023 10:33:54 -0500 Subject: [PATCH 1/2] ipc: disable metrics if proxyd header is present Internally most IPC metrics will start coming from proxyd. To avoid confusion the backend behind it should not publish them if they are already provided for a request by the proxy. --- .../netflix/spectator/ipc/IpcLogEntry.java | 33 ++++++++++--- .../netflix/spectator/ipc/NetflixHeader.java | 26 ++++++---- .../spectator/ipc/IpcLogEntryTest.java | 47 ++++++++++++++++++- 3 files changed, 90 insertions(+), 16 deletions(-) diff --git a/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/IpcLogEntry.java b/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/IpcLogEntry.java index 2e58137a4..85c018fac 100644 --- a/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/IpcLogEntry.java +++ b/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/IpcLogEntry.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2021 Netflix, Inc. + * Copyright 2014-2023 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -93,6 +93,8 @@ public final class IpcLogEntry { private String remoteAddress; private int remotePort; + private boolean disableMetrics; + private final Map additionalTags = new HashMap<>(); private final StringBuilder builder = new StringBuilder(); @@ -154,7 +156,7 @@ public IpcLogEntry withLatency(long latency, TimeUnit unit) { * the request completes it is recommended to call {@link #markEnd()}. */ public IpcLogEntry markStart() { - if (registry != null) { + if (registry != null && !disableMetrics) { inflightId = getInflightId(); int n = logger.inflightRequests(inflightId).incrementAndGet(); registry.distributionSummary(inflightId).record(n); @@ -526,16 +528,17 @@ public IpcLogEntry withResponseContentLength(long length) { */ public IpcLogEntry addRequestHeader(String name, String value) { if (clientAsg == null && name.equalsIgnoreCase(NetflixHeader.ASG.headerName())) { - withClientAsg(value); + withClientAsg(value); } else if (clientZone == null && name.equalsIgnoreCase(NetflixHeader.Zone.headerName())) { withClientZone(value); } else if (clientNode == null && name.equalsIgnoreCase(NetflixHeader.Node.headerName())) { withClientNode(value); } else if (vip == null && name.equalsIgnoreCase(NetflixHeader.Vip.headerName())) { withVip(value); - } else { - this.requestHeaders.add(new Header(name, value)); + } else if (name.equalsIgnoreCase(NetflixHeader.IngressCommonIpcMetrics.headerName())) { + disableMetrics(); } + this.requestHeaders.add(new Header(name, value)); return this; } @@ -552,9 +555,8 @@ public IpcLogEntry addResponseHeader(String name, String value) { withServerNode(value); } else if (endpoint == null && name.equalsIgnoreCase(NetflixHeader.Endpoint.headerName())) { withEndpoint(value); - } else { - this.responseHeaders.add(new Header(name, value)); } + this.responseHeaders.add(new Header(name, value)); return this; } @@ -596,6 +598,15 @@ public IpcLogEntry addTag(String k, String v) { return this; } + /** + * Disable the metrics. The log will still get written, but none of the metrics will get + * updated. + */ + public IpcLogEntry disableMetrics() { + this.disableMetrics = true; + return this; + } + private void putTag(Map tags, Tag tag) { if (tag != null) { tags.put(tag.key(), tag.value()); @@ -707,6 +718,10 @@ private Id getInflightId() { } private void recordClientMetrics() { + if (disableMetrics) { + return; + } + Id clientCall = createCallId(IpcMetric.clientCall.metricName()); PercentileTimer.builder(registry) .withId(clientCall) @@ -727,6 +742,10 @@ private void recordClientMetrics() { } private void recordServerMetrics() { + if (disableMetrics) { + return; + } + Id serverCall = createCallId(IpcMetric.serverCall.metricName()); PercentileTimer.builder(registry) .withId(serverCall) diff --git a/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/NetflixHeader.java b/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/NetflixHeader.java index d7a46545d..7a31e7cf8 100644 --- a/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/NetflixHeader.java +++ b/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/NetflixHeader.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2019 Netflix, Inc. + * Copyright 2014-2023 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -26,24 +26,24 @@ public enum NetflixHeader { * Server group name for the client or the server. It should follow the naming conventions * expected by Frigga. See {@link ServerGroup} for more information. */ - ASG, + ASG("Netflix-ASG"), /** * Availability zone of the client or server instance. */ - Zone, + Zone("Netflix-Zone"), /** * Instance id of the client or server. */ - Node, + Node("Netflix-Node"), /** * Route or route handler for a given path. It should have a fixed cardinality. For HTTP * this would need to come from the server so there is agreement and the client will report * the same value. */ - Endpoint, + Endpoint("Netflix-Endpoint"), /** * VIP that was used to lookup instances for a service when using a client side load balancer. @@ -51,11 +51,21 @@ public enum NetflixHeader { * that would be the VIP used for the DeploymentContextBasedVipAddresses. If multiple VIPs are * used, then the first VIP that caused a given server instance to be selected should be used. * - * For server side load balancers the VIP header should be omitted. + *

For server side load balancers the VIP header should be omitted. */ - Vip; + Vip("Netflix-Vip"), - private final String headerName = "Netflix-" + name(); + /** + * Used to indicate that common IPC metrics are provided by a proxy and do not need to be + * reported locally. Reporting in multiple places can lead to confusing duplication. + */ + IngressCommonIpcMetrics("Netflix-Ingress-Common-IPC-Metrics"); + + private final String headerName; + + private NetflixHeader(String headerName) { + this.headerName = headerName; + } /** Return the fully qualified header name. */ public String headerName() { diff --git a/spectator-ext-ipc/src/test/java/com/netflix/spectator/ipc/IpcLogEntryTest.java b/spectator-ext-ipc/src/test/java/com/netflix/spectator/ipc/IpcLogEntryTest.java index 405f107c3..2b2016e36 100644 --- a/spectator-ext-ipc/src/test/java/com/netflix/spectator/ipc/IpcLogEntryTest.java +++ b/spectator-ext-ipc/src/test/java/com/netflix/spectator/ipc/IpcLogEntryTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2021 Netflix, Inc. + * Copyright 2014-2023 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -718,6 +718,21 @@ public void clientMetricsValidateHttpSuccess() { IpcMetric.validate(registry, true); } + @Test + public void clientMetricsDisbled() { + Registry registry = new DefaultRegistry(); + IpcLogger logger = new IpcLogger(registry, clock, LoggerFactory.getLogger(getClass())); + + logger.createClientEntry() + .withOwner("test") + .disableMetrics() // must be before markStart for inflight + .markStart() + .markEnd() + .log(); + + Assertions.assertEquals(0, registry.stream().count()); + } + @Test public void serverMetricsValidate() { Registry registry = new DefaultRegistry(); @@ -732,6 +747,36 @@ public void serverMetricsValidate() { IpcMetric.validate(registry); } + @Test + public void serverMetricsDisabled() { + Registry registry = new DefaultRegistry(); + IpcLogger logger = new IpcLogger(registry, clock, LoggerFactory.getLogger(getClass())); + + logger.createServerEntry() + .withOwner("test") + .disableMetrics() + .markStart() + .markEnd() + .log(); + + Assertions.assertEquals(0, registry.stream().count()); + } + + @Test + public void serverMetricsDisabledViaHeader() { + Registry registry = new DefaultRegistry(); + IpcLogger logger = new IpcLogger(registry, clock, LoggerFactory.getLogger(getClass())); + + logger.createServerEntry() + .withOwner("test") + .addRequestHeader("netflix-ingress-common-ipc-metrics", "true") + .markStart() + .markEnd() + .log(); + + Assertions.assertEquals(0, registry.stream().count()); + } + @Test public void endpointUnknownIfNotSet() { Registry registry = new DefaultRegistry(); From 5928cd1c7a1934b69ccfb10158151024d349a44a Mon Sep 17 00:00:00 2001 From: Brian Harrington Date: Mon, 9 Oct 2023 10:41:04 -0500 Subject: [PATCH 2/2] fix checkstyle issue --- .../src/main/java/com/netflix/spectator/ipc/NetflixHeader.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/NetflixHeader.java b/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/NetflixHeader.java index 7a31e7cf8..ca74df79c 100644 --- a/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/NetflixHeader.java +++ b/spectator-ext-ipc/src/main/java/com/netflix/spectator/ipc/NetflixHeader.java @@ -63,7 +63,7 @@ public enum NetflixHeader { private final String headerName; - private NetflixHeader(String headerName) { + NetflixHeader(String headerName) { this.headerName = headerName; }