ipc: reset disable flag when reusing entries (#1095)

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 · Nov 18, 2023 · 8363bc9 · 8363bc9
1 parent 2631305
commit 8363bc9
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 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
@@ -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<String, String> 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
@@ -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();