From 74f30dd8e46d2dfc956e4ed81574bb4c89ece578 Mon Sep 17 00:00:00 2001 From: brharrington Date: Mon, 9 Oct 2023 10:48:31 -0500 Subject: [PATCH] ipc: disable metrics if proxyd header is present (#1085) 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..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 @@ -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; + + 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();