From 054d2fe73615dc42f091976f932a6338a21fcec2 Mon Sep 17 00:00:00 2001 From: Brian Harrington Date: Sat, 18 Nov 2023 10:21:36 -0600 Subject: [PATCH] ipc: reset disable flag when reusing entries Before if there was a disabled request that was successful, then all subsequent requests that reuse the entry would also be disabled. Also moves the finalization of some of the fields to a helper that is always called to avoid different values going to the access log if metrics are disabled. --- .../netflix/spectator/ipc/IpcLogEntry.java | 48 ++++++++----------- .../spectator/ipc/IpcLogEntryTest.java | 22 +++++++++ 2 files changed, 43 insertions(+), 27 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 85c018fac..bdd4c8751 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 @@ -535,13 +535,18 @@ public IpcLogEntry addRequestHeader(String name, String value) { withClientNode(value); } else if (vip == null && name.equalsIgnoreCase(NetflixHeader.Vip.headerName())) { withVip(value); - } else if (name.equalsIgnoreCase(NetflixHeader.IngressCommonIpcMetrics.headerName())) { + } else if (isMeshRequest(name, value)) { disableMetrics(); } this.requestHeaders.add(new Header(name, value)); return this; } + private boolean isMeshRequest(String name, String value) { + return name.equalsIgnoreCase(NetflixHeader.IngressCommonIpcMetrics.headerName()) + && "true".equalsIgnoreCase(value); + } + /** * Add a response header value. For special headers in {@link NetflixHeader} it will * automatically fill in the more specific fields based on the header values. @@ -620,38 +625,24 @@ private void putTag(Map tags, String k, String v) { } } - private IpcResult getResult() { + private void finalizeFields() { + // Do final checks and update fields that haven't been explicitly set if needed + // before logging. if (result == null) { result = status == null ? IpcResult.success : status.result(); } - return result; - } - - private IpcStatus getStatus() { if (status == null) { status = (result == IpcResult.success) ? IpcStatus.success : IpcStatus.unexpected_error; } - return status; - } - - private IpcAttempt getAttempt() { if (attempt == null) { attempt = IpcAttempt.forAttemptNumber(1); } - return attempt; - } - - private IpcAttemptFinal getAttemptFinal() { if (attemptFinal == null) { attemptFinal = IpcAttemptFinal.is_true; } - return attemptFinal; - } - - private String getEndpoint() { - return (endpoint == null) - ? (path == null || httpStatus == 404) ? "unknown" : PathSanitizer.sanitize(path) - : endpoint; + if (endpoint == null) { + endpoint = (path == null || httpStatus == 404) ? "unknown" : PathSanitizer.sanitize(path); + } } private boolean isClient() { @@ -669,13 +660,13 @@ private Id createCallId(String name) { // Required for both client and server putTag(tags, IpcTagKey.owner.key(), owner); - putTag(tags, getResult()); - putTag(tags, getStatus()); + putTag(tags, result); + putTag(tags, status); if (isClient()) { // Required for client, should be null on server - putTag(tags, getAttempt()); - putTag(tags, getAttemptFinal()); + putTag(tags, attempt); + putTag(tags, attemptFinal); // Optional for client putTag(tags, IpcTagKey.serverApp.key(), serverApp); @@ -689,7 +680,7 @@ private Id createCallId(String name) { } // Optional for both client and server - putTag(tags, IpcTagKey.endpoint.key(), getEndpoint()); + putTag(tags, IpcTagKey.endpoint.key(), endpoint); putTag(tags, IpcTagKey.vip.key(), vip); putTag(tags, IpcTagKey.protocol.key(), protocol); putTag(tags, IpcTagKey.statusDetail.key(), statusDetail); @@ -771,6 +762,7 @@ private void recordServerMetrics() { */ public void log() { if (logger != null) { + finalizeFields(); if (registry != null) { if (isClient()) { recordClientMetrics(); @@ -898,6 +890,7 @@ void populateMDC() { @Override public String toString() { + finalizeFields(); return new JsonStringBuilder(builder) .startObject() .addField("owner", owner) @@ -906,7 +899,7 @@ public String toString() { .addField("protocol", protocol) .addField("uri", uri) .addField("path", path) - .addField("endpoint", getEndpoint()) + .addField("endpoint", endpoint) .addField("vip", vip) .addField("clientRegion", clientRegion) .addField("clientZone", clientZone) @@ -982,6 +975,7 @@ void reset() { responseHeaders.clear(); remoteAddress = null; remotePort = -1; + disableMetrics = false; additionalTags.clear(); builder.delete(0, builder.length()); inflightId = null; 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 2b2016e36..d12bcac69 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 @@ -777,6 +777,28 @@ public void serverMetricsDisabledViaHeader() { Assertions.assertEquals(0, registry.stream().count()); } + @Test + public void serverMetricsDisabledReuseEntry() { + 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()); + + logger.createServerEntry() + .withOwner("test") + .addRequestHeader("netflix-ingress-common-ipc-metrics", "false") + .markStart() + .markEnd() + .log(); + Assertions.assertEquals(3, registry.stream().count()); + } + @Test public void endpointUnknownIfNotSet() { Registry registry = new DefaultRegistry();