Skip to content

Commit

Permalink
ipc: reset disable flag when reusing entries (#1095)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
brharrington authored Nov 18, 2023
1 parent 2631305 commit 8363bc9
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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() {
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -771,6 +762,7 @@ private void recordServerMetrics() {
*/
public void log() {
if (logger != null) {
finalizeFields();
if (registry != null) {
if (isClient()) {
recordClientMetrics();
Expand Down Expand Up @@ -898,6 +890,7 @@ void populateMDC() {

@Override
public String toString() {
finalizeFields();
return new JsonStringBuilder(builder)
.startObject()
.addField("owner", owner)
Expand All @@ -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)
Expand Down Expand Up @@ -982,6 +975,7 @@ void reset() {
responseHeaders.clear();
remoteAddress = null;
remotePort = -1;
disableMetrics = false;
additionalTags.clear();
builder.delete(0, builder.length());
inflightId = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 8363bc9

Please sign in to comment.