From 62e2ba95c148982def4931bbe1f174976959da30 Mon Sep 17 00:00:00 2001 From: Andre Onuki Date: Thu, 14 Dec 2023 09:42:44 -0500 Subject: [PATCH 1/4] Revert "NEWRELIC-4831 Clean up deprecated attributes (#1125)" This reverts commit 4e36bc58a660d2f50b1f515c512ec055bf25d90e. --- .../agent/instrumentation/akka/http/AkkaHttpRoutesTest.java | 2 +- .../agent/instrumentation/akka/http/AkkaHttpRoutesTest.java | 2 +- .../java/io/grpc/internal/ServerStream_Instrumentation.java | 2 ++ .../grpc-1.22.0/src/test/java/ValidationHelper.java | 4 ++++ .../java/io/grpc/internal/ServerStream_Instrumentation.java | 2 ++ .../grpc-1.30.0/src/test/java/ValidationHelper.java | 4 ++++ .../java/io/grpc/internal/ServerStream_Instrumentation.java | 2 ++ .../grpc-1.4.0/src/test/java/ValidationHelper.java | 4 ++++ .../java/com/nr/agent/instrumentation/grpc/GrpcUtil.java | 4 ++-- .../grpc-1.40.0/src/test/java/ValidationHelper.java | 4 ++++ .../java/com/newrelic/agent/attributes/AttributeNames.java | 2 ++ .../newrelic/agent/dispatchers/WebRequestDispatcher.java | 3 +++ .../newrelic/agent/service/analytics/SpanEventFactory.java | 3 +++ .../agent/service/analytics/TracerToSpanEventTest.java | 6 ++++++ 14 files changed, 40 insertions(+), 4 deletions(-) diff --git a/instrumentation/akka-http-2.11_2.4.5/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java b/instrumentation/akka-http-2.11_2.4.5/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java index f88a9c51bd..de24a030ce 100644 --- a/instrumentation/akka-http-2.11_2.4.5/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java +++ b/instrumentation/akka-http-2.11_2.4.5/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java @@ -1650,7 +1650,7 @@ private void assertResponseCodeOnTxEvents(Collection transacti Assert.assertNotNull(transactionEvents); Assert.assertEquals(expectedSize, transactionEvents.size()); for (TransactionEvent transactionEvent : transactionEvents) { - String httpResponseCode = String.valueOf(transactionEvent.getAttributes().get("http.statusCode")); + String httpResponseCode = String.valueOf(transactionEvent.getAttributes().get("httpResponseCode")); Assert.assertNotNull(httpResponseCode); Assert.assertEquals(expectedResponseCode, httpResponseCode); } diff --git a/instrumentation/akka-http-2.13_10.1.8/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java b/instrumentation/akka-http-2.13_10.1.8/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java index c4089f8fa8..15f5eeed77 100644 --- a/instrumentation/akka-http-2.13_10.1.8/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java +++ b/instrumentation/akka-http-2.13_10.1.8/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java @@ -1646,7 +1646,7 @@ private void assertResponseCodeOnTxEvents(Collection transacti Assert.assertNotNull(transactionEvents); Assert.assertEquals(expectedSize, transactionEvents.size()); for (TransactionEvent transactionEvent : transactionEvents) { - String httpResponseCode = String.valueOf(transactionEvent.getAttributes().get("http.statusCode")); + String httpResponseCode = String.valueOf(transactionEvent.getAttributes().get("httpResponseCode")); Assert.assertNotNull(httpResponseCode); Assert.assertEquals(expectedResponseCode, httpResponseCode); } diff --git a/instrumentation/grpc-1.22.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java b/instrumentation/grpc-1.22.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java index 9833b4c404..e6128f7b64 100644 --- a/instrumentation/grpc-1.22.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java +++ b/instrumentation/grpc-1.22.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java @@ -39,6 +39,7 @@ public void close(Status status, Metadata trailers) { if (status != null) { int statusCode = status.getCode().value(); + NewRelic.addCustomParameter("response.status", statusCode); NewRelic.addCustomParameter("http.statusCode", statusCode); NewRelic.addCustomParameter("http.statusText", status.getDescription()); @@ -69,6 +70,7 @@ public void cancel(Status status) { if (status != null) { int statusCode = status.getCode().value(); + NewRelic.addCustomParameter("response.status", statusCode); NewRelic.addCustomParameter("http.statusCode", statusCode); NewRelic.addCustomParameter("http.statusText", status.getDescription()); if (GrpcConfig.errorsEnabled && status.getCause() != null) { diff --git a/instrumentation/grpc-1.22.0/src/test/java/ValidationHelper.java b/instrumentation/grpc-1.22.0/src/test/java/ValidationHelper.java index a6beb20bce..974bed325e 100644 --- a/instrumentation/grpc-1.22.0/src/test/java/ValidationHelper.java +++ b/instrumentation/grpc-1.22.0/src/test/java/ValidationHelper.java @@ -68,6 +68,7 @@ static void validateGrpcInteraction(TestServer server, String clientTxName, Stri assertTrue(rootSegment.getName().endsWith(fullMethod)); assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); + assertEquals(0, rootSegment.getTracerAttributes().get("response.status")); assertEquals(0, rootSegment.getTracerAttributes().get("http.statusCode")); assertNull(rootSegment.getTracerAttributes().get("http.statusText")); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); @@ -85,6 +86,7 @@ static void validateGrpcInteraction(TestServer server, String clientTxName, Stri assertEquals(name, serverTxEvent.getAttributes().get("sayHelloAfter")); } + assertEquals(0, serverTxEvent.getAttributes().get("response.status")); assertEquals(0, serverTxEvent.getAttributes().get("http.statusCode")); assertEquals("grpc://localhost:" + server.getPort() + "/" + fullMethod, serverTxEvent.getAttributes().get("request.uri")); } @@ -125,6 +127,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN assertTrue(rootSegment.getName().endsWith(fullMethod)); assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); + assertEquals(status, rootSegment.getTracerAttributes().get("response.status")); assertEquals(status, rootSegment.getTracerAttributes().get("http.statusCode")); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); @@ -133,6 +136,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN assertEquals(1, serverTxEvents.size()); TransactionEvent serverTxEvent = serverTxEvents.iterator().next(); assertNotNull(serverTxEvent); + assertEquals(status, serverTxEvent.getAttributes().get("response.status")); assertEquals(status, serverTxEvent.getAttributes().get("http.statusCode")); assertEquals("grpc://localhost:" + server.getPort() + "/" + fullMethod, serverTxEvent.getAttributes().get("request.uri")); } diff --git a/instrumentation/grpc-1.30.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java b/instrumentation/grpc-1.30.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java index 9833b4c404..e6128f7b64 100644 --- a/instrumentation/grpc-1.30.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java +++ b/instrumentation/grpc-1.30.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java @@ -39,6 +39,7 @@ public void close(Status status, Metadata trailers) { if (status != null) { int statusCode = status.getCode().value(); + NewRelic.addCustomParameter("response.status", statusCode); NewRelic.addCustomParameter("http.statusCode", statusCode); NewRelic.addCustomParameter("http.statusText", status.getDescription()); @@ -69,6 +70,7 @@ public void cancel(Status status) { if (status != null) { int statusCode = status.getCode().value(); + NewRelic.addCustomParameter("response.status", statusCode); NewRelic.addCustomParameter("http.statusCode", statusCode); NewRelic.addCustomParameter("http.statusText", status.getDescription()); if (GrpcConfig.errorsEnabled && status.getCause() != null) { diff --git a/instrumentation/grpc-1.30.0/src/test/java/ValidationHelper.java b/instrumentation/grpc-1.30.0/src/test/java/ValidationHelper.java index a6beb20bce..974bed325e 100644 --- a/instrumentation/grpc-1.30.0/src/test/java/ValidationHelper.java +++ b/instrumentation/grpc-1.30.0/src/test/java/ValidationHelper.java @@ -68,6 +68,7 @@ static void validateGrpcInteraction(TestServer server, String clientTxName, Stri assertTrue(rootSegment.getName().endsWith(fullMethod)); assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); + assertEquals(0, rootSegment.getTracerAttributes().get("response.status")); assertEquals(0, rootSegment.getTracerAttributes().get("http.statusCode")); assertNull(rootSegment.getTracerAttributes().get("http.statusText")); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); @@ -85,6 +86,7 @@ static void validateGrpcInteraction(TestServer server, String clientTxName, Stri assertEquals(name, serverTxEvent.getAttributes().get("sayHelloAfter")); } + assertEquals(0, serverTxEvent.getAttributes().get("response.status")); assertEquals(0, serverTxEvent.getAttributes().get("http.statusCode")); assertEquals("grpc://localhost:" + server.getPort() + "/" + fullMethod, serverTxEvent.getAttributes().get("request.uri")); } @@ -125,6 +127,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN assertTrue(rootSegment.getName().endsWith(fullMethod)); assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); + assertEquals(status, rootSegment.getTracerAttributes().get("response.status")); assertEquals(status, rootSegment.getTracerAttributes().get("http.statusCode")); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); @@ -133,6 +136,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN assertEquals(1, serverTxEvents.size()); TransactionEvent serverTxEvent = serverTxEvents.iterator().next(); assertNotNull(serverTxEvent); + assertEquals(status, serverTxEvent.getAttributes().get("response.status")); assertEquals(status, serverTxEvent.getAttributes().get("http.statusCode")); assertEquals("grpc://localhost:" + server.getPort() + "/" + fullMethod, serverTxEvent.getAttributes().get("request.uri")); } diff --git a/instrumentation/grpc-1.4.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java b/instrumentation/grpc-1.4.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java index 1b91851d81..0e84e2c28f 100644 --- a/instrumentation/grpc-1.4.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java +++ b/instrumentation/grpc-1.4.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java @@ -39,6 +39,7 @@ public void close(Status status, Metadata trailers) { if (status != null) { int statusCode = status.getCode().value(); + NewRelic.addCustomParameter("response.status", statusCode); NewRelic.addCustomParameter("http.statusCode", statusCode); NewRelic.addCustomParameter("http.statusText", status.getDescription()); if (GrpcConfig.errorsEnabled && status.getCause() != null) { @@ -68,6 +69,7 @@ public void cancel(Status status) { if (status != null) { int statusCode = status.getCode().value(); + NewRelic.addCustomParameter("response.status", statusCode); NewRelic.addCustomParameter("http.statusCode", statusCode); NewRelic.addCustomParameter("http.statusText", status.getDescription()); if (GrpcConfig.errorsEnabled && status.getCause() != null) { diff --git a/instrumentation/grpc-1.4.0/src/test/java/ValidationHelper.java b/instrumentation/grpc-1.4.0/src/test/java/ValidationHelper.java index fecbc25596..112a362bc7 100644 --- a/instrumentation/grpc-1.4.0/src/test/java/ValidationHelper.java +++ b/instrumentation/grpc-1.4.0/src/test/java/ValidationHelper.java @@ -69,6 +69,7 @@ static void validateGrpcInteraction(TestServer server, String clientTxName, Stri assertTrue(rootSegment.getName().endsWith(fullMethod)); assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); + assertEquals(0, rootSegment.getTracerAttributes().get("response.status")); assertEquals(0, rootSegment.getTracerAttributes().get("http.statusCode")); assertNull(rootSegment.getTracerAttributes().get("http.statusText")); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); @@ -86,6 +87,7 @@ static void validateGrpcInteraction(TestServer server, String clientTxName, Stri assertEquals(name, serverTxEvent.getAttributes().get("sayHelloAfter")); } + assertEquals(0, serverTxEvent.getAttributes().get("response.status")); assertEquals(0, serverTxEvent.getAttributes().get(AttributeNames.HTTP_STATUS_CODE)); assertEquals("grpc://localhost:" + server.getPort() + "/" + fullMethod, serverTxEvent.getAttributes().get("request.uri")); } @@ -126,6 +128,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN assertTrue(rootSegment.getName().endsWith(fullMethod)); assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); + assertEquals(status, rootSegment.getTracerAttributes().get("response.status")); assertEquals(status, rootSegment.getTracerAttributes().get(AttributeNames.HTTP_STATUS_CODE)); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); @@ -134,6 +137,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN assertEquals(1, serverTxEvents.size()); TransactionEvent serverTxEvent = serverTxEvents.iterator().next(); assertNotNull(serverTxEvent); + assertEquals(status, serverTxEvent.getAttributes().get("response.status")); assertEquals(status, serverTxEvent.getAttributes().get(AttributeNames.HTTP_STATUS_CODE)); assertEquals("grpc://localhost:" + server.getPort() + "/" + fullMethod, serverTxEvent.getAttributes().get("request.uri")); } diff --git a/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcUtil.java b/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcUtil.java index c55a2b94a7..2bb214abf3 100644 --- a/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcUtil.java +++ b/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcUtil.java @@ -34,14 +34,14 @@ public static void finalizeTransaction(Token token, Status status, Metadata meta } /** - * Set the http.statusCode attribute and error cause (if applicable) on the transaction + * Set the response.status attribute and error cause (if applicable) on the transaction * when the ServerStream is closed or cancelled. * * @param status The {@link Status} of the completed/cancelled operation */ public static void setServerStreamResponseStatus(Status status) { if (status != null) { - NewRelic.addCustomParameter("http.statusCode", status.getCode().value()); + NewRelic.addCustomParameter("response.status", status.getCode().value()); if (GrpcConfig.errorsEnabled && status.getCause() != null) { // If an error occurred during the close of this server call we should record it NewRelic.noticeError(status.getCause()); diff --git a/instrumentation/grpc-1.40.0/src/test/java/ValidationHelper.java b/instrumentation/grpc-1.40.0/src/test/java/ValidationHelper.java index 024396f030..0fc1ff5eb8 100644 --- a/instrumentation/grpc-1.40.0/src/test/java/ValidationHelper.java +++ b/instrumentation/grpc-1.40.0/src/test/java/ValidationHelper.java @@ -60,6 +60,7 @@ static void validateGrpcInteraction(TestServer server, String clientTxName, Stri assertTrue(rootSegment.getName().endsWith(fullMethod)); assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); + assertEquals(0, rootSegment.getTracerAttributes().get("response.status")); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); // Custom attributes (to test tracing into customer code) @@ -75,6 +76,7 @@ static void validateGrpcInteraction(TestServer server, String clientTxName, Stri assertEquals(name, serverTxEvent.getAttributes().get("sayHelloAfter")); } + assertEquals(0, serverTxEvent.getAttributes().get("response.status")); assertEquals("grpc://localhost:" + server.getPort() + "/" + fullMethod, serverTxEvent.getAttributes().get("request.uri")); } @@ -114,6 +116,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN assertTrue(rootSegment.getName().endsWith(fullMethod)); assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); + assertEquals(status, rootSegment.getTracerAttributes().get("response.status")); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); // Custom attributes (to test tracing into customer code) @@ -121,6 +124,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN assertEquals(1, serverTxEvents.size()); TransactionEvent serverTxEvent = serverTxEvents.iterator().next(); assertNotNull(serverTxEvent); + assertEquals(status, serverTxEvent.getAttributes().get("response.status")); assertEquals("grpc://localhost:" + server.getPort() + "/" + fullMethod, serverTxEvent.getAttributes().get("request.uri")); } diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/attributes/AttributeNames.java b/newrelic-agent/src/main/java/com/newrelic/agent/attributes/AttributeNames.java index 30984d2e0d..a72876b408 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/attributes/AttributeNames.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/attributes/AttributeNames.java @@ -36,6 +36,8 @@ public final class AttributeNames { public static final String HTTP_METHOD = "http.method"; public static final String HTTP_STATUS_CODE = "http.statusCode"; public static final String HTTP_STATUS_TEXT = "http.statusText"; + public static final String HTTP_STATUS = "httpResponseCode"; + public static final String HTTP_STATUS_MESSAGE = "httpResponseMessage"; public static final String LOCK_THREAD_NAME = "jvm.lock_thread_name"; public static final String THREAD_NAME = "jvm.thread_name"; diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/dispatchers/WebRequestDispatcher.java b/newrelic-agent/src/main/java/com/newrelic/agent/dispatchers/WebRequestDispatcher.java index 93d2b2a707..429da22329 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/dispatchers/WebRequestDispatcher.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/dispatchers/WebRequestDispatcher.java @@ -105,11 +105,14 @@ public void transactionActivityWithResponseFinished() { storeResponseContentType(); if (getStatus() > 0) { + // http status is now being recorded as a string + getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS, String.valueOf(getStatus())); // http.statusCode is supposed to be an int getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS_CODE, getStatus()); } if (getStatusMessage() != null) { + getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS_MESSAGE, getStatusMessage()); getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS_TEXT, getStatusMessage()); } diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java b/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java index 7581304fd4..f3c35f3051 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java @@ -231,6 +231,9 @@ public SpanEventFactory setHttpStatusCode(Integer statusCode) { if (filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS_CODE)) { builder.putAgentAttribute(AttributeNames.HTTP_STATUS_CODE, statusCode); } + if (filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS)) { + builder.putAgentAttribute(AttributeNames.HTTP_STATUS, statusCode); + } return this; } diff --git a/newrelic-agent/src/test/java/com/newrelic/agent/service/analytics/TracerToSpanEventTest.java b/newrelic-agent/src/test/java/com/newrelic/agent/service/analytics/TracerToSpanEventTest.java index 97dd485597..4870a274b3 100644 --- a/newrelic-agent/src/test/java/com/newrelic/agent/service/analytics/TracerToSpanEventTest.java +++ b/newrelic-agent/src/test/java/com/newrelic/agent/service/analytics/TracerToSpanEventTest.java @@ -36,6 +36,8 @@ import static com.newrelic.agent.MetricNames.QUEUE_TIME; import static com.newrelic.agent.attributes.AttributeNames.HTTP_REQUEST_PREFIX; +import static com.newrelic.agent.attributes.AttributeNames.HTTP_STATUS; +import static com.newrelic.agent.attributes.AttributeNames.HTTP_STATUS_MESSAGE; import static com.newrelic.agent.attributes.AttributeNames.MESSAGE_REQUEST_PREFIX; import static com.newrelic.agent.attributes.AttributeNames.PORT; import static com.newrelic.agent.attributes.AttributeNames.QUEUE_DURATION; @@ -335,10 +337,14 @@ public void testResponseAttributesAddedToRoot() { int httpResponseCode = 404; String httpResponseMessage = "I cannot find that page, silly"; String contentType = "application/vnd.ms-powerpoint "; + expectedAgentAttributes.put("httpResponseCode", httpResponseCode); + expectedAgentAttributes.put("httpResponseMessage", httpResponseMessage); expectedAgentAttributes.put(RESPONSE_CONTENT_TYPE_PARAMETER_NAME, contentType); SpanEvent expectedSpanEvent = buildExpectedSpanEvent(); + transactionAgentAttributes.put(HTTP_STATUS, httpResponseCode); + transactionAgentAttributes.put(HTTP_STATUS_MESSAGE, httpResponseMessage); transactionAgentAttributes.put(RESPONSE_CONTENT_TYPE_PARAMETER_NAME, contentType); when(txnData.getAgentAttributes()).thenReturn(transactionAgentAttributes); From 07262b56126882067738e29de4628c0be15f7124 Mon Sep 17 00:00:00 2001 From: Andre Onuki Date: Thu, 14 Dec 2023 15:26:17 -0500 Subject: [PATCH 2/4] Reintroduce previously deprecated transaction/span attributes --- .../agent/config/AttributesConfig.java | 11 ++++++ .../agent/config/AttributesConfigImpl.java | 34 ++++++++++++++++++- .../dispatchers/WebRequestDispatcher.java | 24 +++++++++---- .../service/analytics/SpanEventFactory.java | 17 ++++++++-- .../src/main/resources/newrelic.yml | 6 ++++ 5 files changed, 81 insertions(+), 11 deletions(-) diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfig.java b/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfig.java index 3da234bb2e..a8b1fd15ee 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfig.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfig.java @@ -19,4 +19,15 @@ public interface AttributesConfig { List attributesRootExclude(); + /** + * Whether the old http attributes (httpResponseCode, httpResponseMessage) should be sent. + * @since 8.8.1 + */ + boolean isLegacyHttpAttr(); + + /** + * Whether the new http attributes (http.statusCode, http.statusText) should be sent. + * @since 8.8.1 + */ + boolean isStandardHttpAttr(); } diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfigImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfigImpl.java index 019055d3dc..401b5743ea 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfigImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfigImpl.java @@ -8,7 +8,7 @@ package com.newrelic.agent.config; import com.newrelic.agent.Agent; -import com.newrelic.agent.attributes.AttributeNames; +import com.newrelic.agent.bridge.AgentBridge; import java.text.MessageFormat; import java.util.ArrayList; @@ -29,15 +29,37 @@ public class AttributesConfigImpl extends BaseConfig implements AttributesConfig public static final String ATTS_EXCLUDE = "attributes.exclude"; public static final String ATTS_INCLUDE = "attributes.include"; + private static final String HTTP_ATTR_MODE = "http_attribute_mode"; + private static final String HTTP_ATTR_MODE_LEGACY = "legacy"; + private static final String HTTP_ATTR_MODE_STANDARD = "standard"; + private static final String HTTP_ATTR_MODE_BOTH = "both"; + private final boolean enabledRoot; private final List attributesInclude; private final List attributeExclude; + private final boolean legacyHttpAttr; + private final boolean standardHttpAttr; public AttributesConfigImpl(Map pProps) { super(pProps, SYSTEM_PROPERTY_ROOT); enabledRoot = initEnabled(); attributesInclude = initAttributesInclude(); attributeExclude = initAttributesExclude(); + String httpAttributeMode = getProperty(HTTP_ATTR_MODE); + if (HTTP_ATTR_MODE_LEGACY.equalsIgnoreCase(httpAttributeMode)) { + legacyHttpAttr = true; + standardHttpAttr = false; + } else if (HTTP_ATTR_MODE_STANDARD.equalsIgnoreCase(httpAttributeMode)) { + legacyHttpAttr = false; + standardHttpAttr = true; + } else { // defaults to all + if (httpAttributeMode != null && !HTTP_ATTR_MODE_BOTH.equalsIgnoreCase(httpAttributeMode)) { + AgentBridge.getAgent().getLogger().log(Level.WARNING, "Invalid " + HTTP_ATTR_MODE + " config" + + " encountered: " + httpAttributeMode + ". Using default :" + HTTP_ATTR_MODE_BOTH + "."); + } + legacyHttpAttr = true; + standardHttpAttr = true; + } } private boolean initEnabled() { @@ -90,6 +112,16 @@ public boolean isAttsEnabled(AgentConfig config, boolean defaultProp, String... return (toEnable || defaultProp); } + @Override + public boolean isLegacyHttpAttr() { + return legacyHttpAttr; + } + + @Override + public boolean isStandardHttpAttr() { + return standardHttpAttr; + } + private static Boolean getBooleanValue(AgentConfig config, String value) { try { Object inputObj = config.getValue(value); diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/dispatchers/WebRequestDispatcher.java b/newrelic-agent/src/main/java/com/newrelic/agent/dispatchers/WebRequestDispatcher.java index 429da22329..f47cd36b42 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/dispatchers/WebRequestDispatcher.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/dispatchers/WebRequestDispatcher.java @@ -16,8 +16,10 @@ import com.newrelic.agent.bridge.WebResponse; import com.newrelic.agent.config.AgentConfig; import com.newrelic.agent.config.AgentConfigImpl; +import com.newrelic.agent.config.AttributesConfig; import com.newrelic.agent.config.CustomRequestHeaderConfig; import com.newrelic.agent.config.HiddenProperties; +import com.newrelic.agent.config.TransactionEventsConfig; import com.newrelic.agent.config.TransactionTracerConfig; import com.newrelic.agent.service.ServiceFactory; import com.newrelic.agent.servlet.ServletUtils; @@ -27,7 +29,6 @@ import com.newrelic.agent.tracers.servlet.ExternalTimeTracker; import com.newrelic.agent.transaction.TransactionNamer; import com.newrelic.agent.transaction.WebTransactionNamer; -import com.newrelic.agent.util.Strings; import com.newrelic.api.agent.ExtendedRequest; import com.newrelic.api.agent.Request; import com.newrelic.api.agent.Response; @@ -104,16 +105,25 @@ public void transactionActivityWithResponseFinished() { storeMethod(); storeResponseContentType(); + AttributesConfig attributesConfig = ServiceFactory.getConfigService().getDefaultAgentConfig().getAttributesConfig(); if (getStatus() > 0) { - // http status is now being recorded as a string - getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS, String.valueOf(getStatus())); - // http.statusCode is supposed to be an int - getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS_CODE, getStatus()); + if (attributesConfig.isLegacyHttpAttr()) { + // http status is now being recorded as a string + getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS, String.valueOf(getStatus())); + } + if (attributesConfig.isStandardHttpAttr()) { + // http.statusCode is supposed to be an int + getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS_CODE, getStatus()); + } } if (getStatusMessage() != null) { - getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS_MESSAGE, getStatusMessage()); - getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS_TEXT, getStatusMessage()); + if (attributesConfig.isLegacyHttpAttr()) { + getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS_MESSAGE, getStatusMessage()); + } + if (attributesConfig.isStandardHttpAttr()) { + getTransaction().getAgentAttributes().put(AttributeNames.HTTP_STATUS_TEXT, getStatusMessage()); + } } // adding request.uri here includes it in the Transaction event, which also propagates to any Transaction error events diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java b/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java index f3c35f3051..3d5c944d55 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java @@ -11,6 +11,8 @@ import com.newrelic.agent.attributes.AttributeNames; import com.newrelic.agent.attributes.AttributeValidator; import com.newrelic.agent.config.AgentConfig; +import com.newrelic.agent.config.AttributesConfig; +import com.newrelic.agent.config.TransactionEventsConfig; import com.newrelic.agent.database.SqlObfuscator; import com.newrelic.agent.model.AttributeFilter; import com.newrelic.agent.model.SpanCategory; @@ -228,19 +230,28 @@ public SpanEventFactory setHttpComponent(String component) { } public SpanEventFactory setHttpStatusCode(Integer statusCode) { - if (filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS_CODE)) { + AttributesConfig attributesConfig = ServiceFactory.getConfigService().getDefaultAgentConfig().getAttributesConfig(); + if (attributesConfig.isStandardHttpAttr() && + filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS_CODE)) { builder.putAgentAttribute(AttributeNames.HTTP_STATUS_CODE, statusCode); } - if (filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS)) { + if (attributesConfig.isLegacyHttpAttr() && + filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS)) { builder.putAgentAttribute(AttributeNames.HTTP_STATUS, statusCode); } return this; } public SpanEventFactory setHttpStatusText(String statusText) { - if (filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS_TEXT)) { + AttributesConfig attributesConfig = ServiceFactory.getConfigService().getDefaultAgentConfig().getAttributesConfig(); + if (attributesConfig.isStandardHttpAttr() && + filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS_TEXT)) { builder.putAgentAttribute(AttributeNames.HTTP_STATUS_TEXT, statusText); } + if (attributesConfig.isStandardHttpAttr() && + filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS_TEXT)) { + builder.putAgentAttribute(AttributeNames.HTTP_STATUS_MESSAGE, statusText); + } return this; } diff --git a/newrelic-agent/src/main/resources/newrelic.yml b/newrelic-agent/src/main/resources/newrelic.yml index 5b4c271dcf..14ed8a4c9a 100644 --- a/newrelic-agent/src/main/resources/newrelic.yml +++ b/newrelic-agent/src/main/resources/newrelic.yml @@ -172,6 +172,12 @@ common: &default_settings # not be sent to New Relic. #exclude: + + # Defines which sets of http attributes the agent will send: standard, legacy or both (default). + # Having the agent send both sets will increase ingestion. + # Having the agent send only legacy may impact current or future functionality. + http_attribute_mode: both + # Transaction tracer captures deep information about slow # transactions and sends this to the New Relic service once a # minute. Included in the transaction is the exact call sequence of From 6c144c127c7445ad1c57833c8d65f0c0996629fc Mon Sep 17 00:00:00 2001 From: Andre Onuki Date: Fri, 15 Dec 2023 11:59:36 -0500 Subject: [PATCH 3/4] Adding http attr selection to grpc --- .../akka/http/AkkaHttpRoutesTest.java | 4 ++- .../akka/http/AkkaHttpRoutesTest.java | 2 ++ .../instrumentation/grpc/GrpcConfig.java | 10 +++++++ .../ServerStream_Instrumentation.java | 12 ++++++--- .../instrumentation/grpc/GrpcConfig.java | 10 +++++++ .../ServerStream_Instrumentation.java | 12 ++++++--- .../instrumentation/grpc/GrpcConfig.java | 10 +++++++ .../ServerStream_Instrumentation.java | 12 ++++++--- .../instrumentation/grpc/GrpcConfig.java | 10 +++++++ .../agent/instrumentation/grpc/GrpcUtil.java | 14 ++++++++-- .../src/test/java/ValidationHelper.java | 3 +++ .../agent/config/AttributesConfigImpl.java | 27 ++++++++++--------- 12 files changed, 101 insertions(+), 25 deletions(-) diff --git a/instrumentation/akka-http-2.11_2.4.5/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java b/instrumentation/akka-http-2.11_2.4.5/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java index de24a030ce..43d13411e9 100644 --- a/instrumentation/akka-http-2.11_2.4.5/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java +++ b/instrumentation/akka-http-2.11_2.4.5/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java @@ -1650,9 +1650,11 @@ private void assertResponseCodeOnTxEvents(Collection transacti Assert.assertNotNull(transactionEvents); Assert.assertEquals(expectedSize, transactionEvents.size()); for (TransactionEvent transactionEvent : transactionEvents) { - String httpResponseCode = String.valueOf(transactionEvent.getAttributes().get("httpResponseCode")); + String httpResponseCode = (String) transactionEvent.getAttributes().get("httpResponseCode"); Assert.assertNotNull(httpResponseCode); Assert.assertEquals(expectedResponseCode, httpResponseCode); + int statusCode = (Integer) transactionEvent.getAttributes().get("http.statusCode"); + Assert.assertEquals(Integer.parseInt(expectedResponseCode), statusCode); } } diff --git a/instrumentation/akka-http-2.13_10.1.8/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java b/instrumentation/akka-http-2.13_10.1.8/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java index 15f5eeed77..17aebd7a74 100644 --- a/instrumentation/akka-http-2.13_10.1.8/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java +++ b/instrumentation/akka-http-2.13_10.1.8/src/test/java/com/agent/instrumentation/akka/http/AkkaHttpRoutesTest.java @@ -1649,6 +1649,8 @@ private void assertResponseCodeOnTxEvents(Collection transacti String httpResponseCode = String.valueOf(transactionEvent.getAttributes().get("httpResponseCode")); Assert.assertNotNull(httpResponseCode); Assert.assertEquals(expectedResponseCode, httpResponseCode); + int statusCode = (Integer) transactionEvent.getAttributes().get("http.statusCode"); + Assert.assertEquals(Integer.parseInt(expectedResponseCode), statusCode); } } diff --git a/instrumentation/grpc-1.22.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java b/instrumentation/grpc-1.22.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java index 9ff40de150..77b92868f1 100644 --- a/instrumentation/grpc-1.22.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java +++ b/instrumentation/grpc-1.22.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java @@ -15,6 +15,16 @@ public class GrpcConfig { public static final boolean errorsEnabled = NewRelic.getAgent().getConfig().getValue("grpc.errors.enabled", true); + public static final boolean HTTP_ATTR_LEGACY; + public static final boolean HTTP_ATTR_STANDARD; + + static { + String attrMode = NewRelic.getAgent().getConfig().getValue("attributes.http_attribute_mode", "both"); + // legacy is only disabled when standard is selected. + HTTP_ATTR_LEGACY = !"standard".equalsIgnoreCase(attrMode); + // standard is only disabled when legacy is selected. + HTTP_ATTR_STANDARD = !"legacy".equalsIgnoreCase(attrMode); + } private GrpcConfig() { } diff --git a/instrumentation/grpc-1.22.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java b/instrumentation/grpc-1.22.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java index e6128f7b64..f6f5808352 100644 --- a/instrumentation/grpc-1.22.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java +++ b/instrumentation/grpc-1.22.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java @@ -70,9 +70,15 @@ public void cancel(Status status) { if (status != null) { int statusCode = status.getCode().value(); - NewRelic.addCustomParameter("response.status", statusCode); - NewRelic.addCustomParameter("http.statusCode", statusCode); - NewRelic.addCustomParameter("http.statusText", status.getDescription()); + String statusMessage = status.getDescription(); + if (GrpcConfig.HTTP_ATTR_LEGACY) { + NewRelic.addCustomParameter("response.status", statusCode); + NewRelic.addCustomParameter("response.statusMessage", statusMessage); + } + if (GrpcConfig.HTTP_ATTR_STANDARD) { + NewRelic.addCustomParameter("http.statusCode", statusCode); + NewRelic.addCustomParameter("http.statusText", statusMessage); + } if (GrpcConfig.errorsEnabled && status.getCause() != null) { // If an error occurred during the close of this server call we should record it NewRelic.noticeError(status.getCause()); diff --git a/instrumentation/grpc-1.30.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java b/instrumentation/grpc-1.30.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java index 9ff40de150..77b92868f1 100644 --- a/instrumentation/grpc-1.30.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java +++ b/instrumentation/grpc-1.30.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java @@ -15,6 +15,16 @@ public class GrpcConfig { public static final boolean errorsEnabled = NewRelic.getAgent().getConfig().getValue("grpc.errors.enabled", true); + public static final boolean HTTP_ATTR_LEGACY; + public static final boolean HTTP_ATTR_STANDARD; + + static { + String attrMode = NewRelic.getAgent().getConfig().getValue("attributes.http_attribute_mode", "both"); + // legacy is only disabled when standard is selected. + HTTP_ATTR_LEGACY = !"standard".equalsIgnoreCase(attrMode); + // standard is only disabled when legacy is selected. + HTTP_ATTR_STANDARD = !"legacy".equalsIgnoreCase(attrMode); + } private GrpcConfig() { } diff --git a/instrumentation/grpc-1.30.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java b/instrumentation/grpc-1.30.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java index e6128f7b64..06d74add7a 100644 --- a/instrumentation/grpc-1.30.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java +++ b/instrumentation/grpc-1.30.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java @@ -39,9 +39,15 @@ public void close(Status status, Metadata trailers) { if (status != null) { int statusCode = status.getCode().value(); - NewRelic.addCustomParameter("response.status", statusCode); - NewRelic.addCustomParameter("http.statusCode", statusCode); - NewRelic.addCustomParameter("http.statusText", status.getDescription()); + String statusMessage = status.getDescription(); + if (GrpcConfig.HTTP_ATTR_LEGACY) { + NewRelic.addCustomParameter("response.status", statusCode); + NewRelic.addCustomParameter("response.statusMessage", statusMessage); + } + if (GrpcConfig.HTTP_ATTR_STANDARD) { + NewRelic.addCustomParameter("http.statusCode", statusCode); + NewRelic.addCustomParameter("http.statusText", statusMessage); + } if (GrpcConfig.errorsEnabled && status.getCause() != null) { // If an error occurred during the close of this server call we should record it diff --git a/instrumentation/grpc-1.4.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java b/instrumentation/grpc-1.4.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java index 9ff40de150..77b92868f1 100644 --- a/instrumentation/grpc-1.4.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java +++ b/instrumentation/grpc-1.4.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java @@ -15,6 +15,16 @@ public class GrpcConfig { public static final boolean errorsEnabled = NewRelic.getAgent().getConfig().getValue("grpc.errors.enabled", true); + public static final boolean HTTP_ATTR_LEGACY; + public static final boolean HTTP_ATTR_STANDARD; + + static { + String attrMode = NewRelic.getAgent().getConfig().getValue("attributes.http_attribute_mode", "both"); + // legacy is only disabled when standard is selected. + HTTP_ATTR_LEGACY = !"standard".equalsIgnoreCase(attrMode); + // standard is only disabled when legacy is selected. + HTTP_ATTR_STANDARD = !"legacy".equalsIgnoreCase(attrMode); + } private GrpcConfig() { } diff --git a/instrumentation/grpc-1.4.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java b/instrumentation/grpc-1.4.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java index 0e84e2c28f..18b777f44d 100644 --- a/instrumentation/grpc-1.4.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java +++ b/instrumentation/grpc-1.4.0/src/main/java/io/grpc/internal/ServerStream_Instrumentation.java @@ -39,9 +39,15 @@ public void close(Status status, Metadata trailers) { if (status != null) { int statusCode = status.getCode().value(); - NewRelic.addCustomParameter("response.status", statusCode); - NewRelic.addCustomParameter("http.statusCode", statusCode); - NewRelic.addCustomParameter("http.statusText", status.getDescription()); + String statusMessage = status.getDescription(); + if (GrpcConfig.HTTP_ATTR_LEGACY) { + NewRelic.addCustomParameter("response.status", statusCode); + NewRelic.addCustomParameter("response.statusMessage", statusMessage); + } + if (GrpcConfig.HTTP_ATTR_STANDARD) { + NewRelic.addCustomParameter("http.statusCode", statusCode); + NewRelic.addCustomParameter("http.statusText", statusMessage); + } if (GrpcConfig.errorsEnabled && status.getCause() != null) { // If an error occurred during the close of this server call we should record it NewRelic.noticeError(status.getCause()); diff --git a/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java b/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java index 56600551fb..7237b5508d 100644 --- a/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java +++ b/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcConfig.java @@ -15,6 +15,16 @@ public class GrpcConfig { public static final boolean errorsEnabled = NewRelic.getAgent().getConfig().getValue("grpc.errors.enabled", true); + public static final boolean HTTP_ATTR_LEGACY; + public static final boolean HTTP_ATTR_STANDARD; + + static { + String attrMode = NewRelic.getAgent().getConfig().getValue("attributes.http_attribute_mode", "both"); + // legacy is only disabled when standard is selected. + HTTP_ATTR_LEGACY = !"standard".equalsIgnoreCase(attrMode); + // standard is only disabled when legacy is selected. + HTTP_ATTR_STANDARD = !"legacy".equalsIgnoreCase(attrMode); + } private GrpcConfig() { } diff --git a/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcUtil.java b/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcUtil.java index 2bb214abf3..4239bb7365 100644 --- a/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcUtil.java +++ b/instrumentation/grpc-1.40.0/src/main/java/com/nr/agent/instrumentation/grpc/GrpcUtil.java @@ -34,14 +34,24 @@ public static void finalizeTransaction(Token token, Status status, Metadata meta } /** - * Set the response.status attribute and error cause (if applicable) on the transaction + * Set the http status code attribute and error cause (if applicable) on the transaction * when the ServerStream is closed or cancelled. * * @param status The {@link Status} of the completed/cancelled operation */ public static void setServerStreamResponseStatus(Status status) { + if (status != null) { - NewRelic.addCustomParameter("response.status", status.getCode().value()); + String statusMessage = status.getDescription(); + int value = status.getCode().value(); // code should not be null + if (GrpcConfig.HTTP_ATTR_LEGACY) { + NewRelic.addCustomParameter("response.status", value); + NewRelic.addCustomParameter("response.statusMessage", statusMessage); + } + if (GrpcConfig.HTTP_ATTR_STANDARD) { + NewRelic.addCustomParameter("http.statusCode", value); + NewRelic.addCustomParameter("http.statusText", statusMessage); + } if (GrpcConfig.errorsEnabled && status.getCause() != null) { // If an error occurred during the close of this server call we should record it NewRelic.noticeError(status.getCause()); diff --git a/instrumentation/grpc-1.40.0/src/test/java/ValidationHelper.java b/instrumentation/grpc-1.40.0/src/test/java/ValidationHelper.java index 0fc1ff5eb8..79aeae6084 100644 --- a/instrumentation/grpc-1.40.0/src/test/java/ValidationHelper.java +++ b/instrumentation/grpc-1.40.0/src/test/java/ValidationHelper.java @@ -61,6 +61,7 @@ static void validateGrpcInteraction(TestServer server, String clientTxName, Stri assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); assertEquals(0, rootSegment.getTracerAttributes().get("response.status")); + assertEquals(0, rootSegment.getTracerAttributes().get("http.statusCode")); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); // Custom attributes (to test tracing into customer code) @@ -117,6 +118,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN assertEquals(1, rootSegment.getCallCount()); assertEquals(fullMethod, rootSegment.getTracerAttributes().get("request.method")); assertEquals(status, rootSegment.getTracerAttributes().get("response.status")); + assertEquals(status, rootSegment.getTracerAttributes().get("http.statusCode")); assertEquals(grpcType, rootSegment.getTracerAttributes().get("grpc.type")); // Custom attributes (to test tracing into customer code) @@ -125,6 +127,7 @@ static void validateExceptionGrpcInteraction(TestServer server, String clientTxN TransactionEvent serverTxEvent = serverTxEvents.iterator().next(); assertNotNull(serverTxEvent); assertEquals(status, serverTxEvent.getAttributes().get("response.status")); + assertEquals(status, serverTxEvent.getAttributes().get("http.statusCode")); assertEquals("grpc://localhost:" + server.getPort() + "/" + fullMethod, serverTxEvent.getAttributes().get("request.uri")); } diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfigImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfigImpl.java index 401b5743ea..ff1b3dac27 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfigImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/config/AttributesConfigImpl.java @@ -46,19 +46,20 @@ public AttributesConfigImpl(Map pProps) { attributesInclude = initAttributesInclude(); attributeExclude = initAttributesExclude(); String httpAttributeMode = getProperty(HTTP_ATTR_MODE); - if (HTTP_ATTR_MODE_LEGACY.equalsIgnoreCase(httpAttributeMode)) { - legacyHttpAttr = true; - standardHttpAttr = false; - } else if (HTTP_ATTR_MODE_STANDARD.equalsIgnoreCase(httpAttributeMode)) { - legacyHttpAttr = false; - standardHttpAttr = true; - } else { // defaults to all - if (httpAttributeMode != null && !HTTP_ATTR_MODE_BOTH.equalsIgnoreCase(httpAttributeMode)) { - AgentBridge.getAgent().getLogger().log(Level.WARNING, "Invalid " + HTTP_ATTR_MODE + " config" + - " encountered: " + httpAttributeMode + ". Using default :" + HTTP_ATTR_MODE_BOTH + "."); - } - legacyHttpAttr = true; - standardHttpAttr = true; + + boolean standardMode = HTTP_ATTR_MODE_STANDARD.equalsIgnoreCase(httpAttributeMode); + boolean legacyMode = HTTP_ATTR_MODE_LEGACY.equalsIgnoreCase(httpAttributeMode); + + // legacy is only disabled when mode is standard + legacyHttpAttr = !standardMode; + // standard is only disabled when mode is legacy + standardHttpAttr = !legacyMode; + + // logging invalid http attr mode + if (httpAttributeMode != null && !legacyMode && !standardMode && + !HTTP_ATTR_MODE_BOTH.equalsIgnoreCase(httpAttributeMode)) { + AgentBridge.getAgent().getLogger().log(Level.WARNING, "Invalid " + HTTP_ATTR_MODE + " config" + + " encountered: " + httpAttributeMode + ". Using default :" + HTTP_ATTR_MODE_BOTH + "."); } } From 52408c3040f2cd300cad5d64affbaf76d3a8502d Mon Sep 17 00:00:00 2001 From: Andre Onuki Date: Fri, 15 Dec 2023 15:32:56 -0500 Subject: [PATCH 4/4] Fixing add unit tests to legacy/std http attrs --- .../service/analytics/SpanEventFactory.java | 4 +- .../analytics/SpanEventFactoryTest.java | 67 +++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java b/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java index 3d5c944d55..02464d849a 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/SpanEventFactory.java @@ -248,8 +248,8 @@ public SpanEventFactory setHttpStatusText(String statusText) { filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS_TEXT)) { builder.putAgentAttribute(AttributeNames.HTTP_STATUS_TEXT, statusText); } - if (attributesConfig.isStandardHttpAttr() && - filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS_TEXT)) { + if (attributesConfig.isLegacyHttpAttr() && + filter.shouldIncludeAgentAttribute(appName, AttributeNames.HTTP_STATUS_MESSAGE)) { builder.putAgentAttribute(AttributeNames.HTTP_STATUS_MESSAGE, statusText); } return this; diff --git a/newrelic-agent/src/test/java/com/newrelic/agent/service/analytics/SpanEventFactoryTest.java b/newrelic-agent/src/test/java/com/newrelic/agent/service/analytics/SpanEventFactoryTest.java index 0ffaf80cf9..a8e01f8c68 100644 --- a/newrelic-agent/src/test/java/com/newrelic/agent/service/analytics/SpanEventFactoryTest.java +++ b/newrelic-agent/src/test/java/com/newrelic/agent/service/analytics/SpanEventFactoryTest.java @@ -32,6 +32,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.mockito.Mockito.RETURNS_DEEP_STUBS; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -113,6 +114,7 @@ public void shouldNotSetSpanErrorWhenFiltered() { @Test public void shouldSetHttpParameters() { + mockAttributeConfig(HttpAttrMode.BOTH); HttpParameters mockParameters = mock(HttpParameters.class); when(mockParameters.getLibrary()).thenReturn("library"); when(mockParameters.getProcedure()).thenReturn("procedure"); @@ -138,13 +140,34 @@ public void doesNotSetHttpAgentAttributesWhenFiltering() { @Test public void shouldSetStatusCode() { + mockAttributeConfig(HttpAttrMode.BOTH); SpanEvent spanEvent = spanEventFactory.setHttpStatusCode(418).build(); assertEquals(418, spanEvent.getAgentAttributes().get("http.statusCode")); + assertEquals(418, spanEvent.getAgentAttributes().get("httpResponseCode")); + } + + @Test + public void shouldSetStandardStatusCode() { + mockAttributeConfig(HttpAttrMode.STANDARD); + SpanEvent spanEvent = spanEventFactory.setHttpStatusCode(418).build(); + + assertEquals(418, spanEvent.getAgentAttributes().get("http.statusCode")); + assertNull(spanEvent.getAgentAttributes().get("httpResponseCode")); + } + + @Test + public void shouldSetLegacyStatusCode() { + mockAttributeConfig(HttpAttrMode.LEGACY); + SpanEvent spanEvent = spanEventFactory.setHttpStatusCode(418).build(); + + assertNull(spanEvent.getAgentAttributes().get("http.statusCode")); + assertEquals(418, spanEvent.getAgentAttributes().get("httpResponseCode")); } @Test public void shouldNotSetStatusCodeWhenFiltering() { + mockAttributeConfig(HttpAttrMode.BOTH); SpanEventFactory factory = new SpanEventFactory("blerb", new PassNothingAttributeFilter(), DEFAULT_SYSTEM_TIMESTAMP_SUPPLIER); SpanEvent spanEvent = factory.setHttpStatusCode(418).build(); @@ -160,17 +183,39 @@ public void shouldNotSetNullStatusCode() { @Test public void shouldSetStatusText() { + mockAttributeConfig(HttpAttrMode.BOTH); SpanEvent spanEvent = spanEventFactory.setHttpStatusText("I'm a teapot.").build(); assertEquals("I'm a teapot.", spanEvent.getAgentAttributes().get("http.statusText")); + assertEquals("I'm a teapot.", spanEvent.getAgentAttributes().get("httpResponseMessage")); + } + + @Test + public void shouldSetStandardStatusText() { + mockAttributeConfig(HttpAttrMode.STANDARD); + SpanEvent spanEvent = spanEventFactory.setHttpStatusText("I'm a teapot.").build(); + + assertEquals("I'm a teapot.", spanEvent.getAgentAttributes().get("http.statusText")); + assertNull(spanEvent.getAgentAttributes().get("httpResponseMessage")); + } + + @Test + public void shouldSetLegacyStatusText() { + mockAttributeConfig(HttpAttrMode.LEGACY); + SpanEvent spanEvent = spanEventFactory.setHttpStatusText("I'm a teapot.").build(); + + assertNull(spanEvent.getAgentAttributes().get("http.statusText")); + assertEquals("I'm a teapot.", spanEvent.getAgentAttributes().get("httpResponseMessage")); } @Test public void shouldNotSetStatusTextWhenFiltering() { + mockAttributeConfig(HttpAttrMode.BOTH); SpanEventFactory factory = new SpanEventFactory("blerb", new PassNothingAttributeFilter(), DEFAULT_SYSTEM_TIMESTAMP_SUPPLIER); SpanEvent spanEvent = factory.setHttpStatusText("I'm a teapot.").build(); assertNull(spanEvent.getAgentAttributes().get("http.statusText")); + assertNull(spanEvent.getAgentAttributes().get("httpResponseMessage")); } @Test @@ -178,6 +223,7 @@ public void shouldNotSetNullStatusText() { SpanEvent spanEvent = spanEventFactory.setHttpStatusText(null).build(); assertFalse(spanEvent.getAgentAttributes().containsKey("http.statusText")); + assertFalse(spanEvent.getAgentAttributes().containsKey("httpResponseMessage")); } @Test @@ -278,4 +324,25 @@ public boolean shouldIncludeAgentAttribute(String appName, String attributeName) return Collections.emptyMap(); } } + + /** + * These should never be both false. + */ + private void mockAttributeConfig(HttpAttrMode httpAttrMode) { + MockServiceManager serviceManager = new MockServiceManager(); + AgentConfig agentConfig = mock(AgentConfig.class, RETURNS_DEEP_STUBS); + serviceManager.setConfigService(new MockConfigService(agentConfig)); + + when(agentConfig.getAttributesConfig().isStandardHttpAttr()) + .thenReturn(httpAttrMode != HttpAttrMode.LEGACY); + + when(agentConfig.getAttributesConfig().isLegacyHttpAttr()) + .thenReturn(httpAttrMode != HttpAttrMode.STANDARD); + } + + private enum HttpAttrMode { + BOTH, + STANDARD, + LEGACY, + } }