From dc27a6bc633af67220592e706084f5c8d421a9ff Mon Sep 17 00:00:00 2001 From: Oren Ben-Meir Date: Tue, 1 Oct 2024 14:12:45 -0400 Subject: [PATCH 1/4] httpurlconnection metric state use weakref tracers --- .../httpurlconnection/MetricState.java | 75 ++++++++++++------- 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java b/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java index 9630613088..225a228ef5 100644 --- a/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java +++ b/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java @@ -16,6 +16,7 @@ import com.newrelic.api.agent.HttpParameters; import com.newrelic.api.agent.OutboundHeaders; +import java.lang.ref.WeakReference; import java.net.HttpURLConnection; import java.net.URI; import java.net.UnknownHostException; @@ -48,8 +49,8 @@ public class MetricState { // the guids for these tracers are swapped so the DT is attached to the tracer that // has the external - private TracedMethod dtTracer; - private TracedMethod externalTracer; + private WeakReference dtTracerRef; + private WeakReference externalTracerRef; private boolean externalReported = false; /** @@ -79,12 +80,12 @@ public void nonNetworkPreamble(boolean isConnected, HttpURLConnection connection * @param tracer traced method that will be the external */ public void inboundPreamble(boolean isConnected, HttpURLConnection connection, TracedMethod tracer) { - if (externalReported || externalTracer != null) { + if (externalReported || getExternalTracer() != null) { // another method already ran the preamble return; } Transaction tx = AgentBridge.getAgent().getTransaction(false); - externalTracer = tracer; + setExternalTracer(tracer); if (!isConnected && tracer.isMetricProducer() && tx != null) { addOutboundHeadersIfNotAdded(connection); @@ -102,7 +103,7 @@ public void inboundPreamble(boolean isConnected, HttpURLConnection connection, T public void inboundPostamble(HttpURLConnection connection, int responseCode, String responseMessage, Ops operation, TracedMethod tracer) { // make sure that only the method that first invoked inboundPreamble runs this method - if (externalReported || externalTracer != tracer) { + if (externalReported || getExternalTracer() != tracer) { return; } Transaction tx = AgentBridge.getAgent().getTransaction(false); @@ -112,7 +113,8 @@ public void inboundPostamble(HttpURLConnection connection, int responseCode, Str } public void handleException(TracedMethod tracer, Exception e) { - if (externalTracer != tracer) { + TracedMethod externalTracer = getExternalTracer(); + if (externalTracer != tracer || externalTracer == null) { return; } @@ -125,8 +127,8 @@ public void handleException(TracedMethod tracer, Exception e) { externalReported = true; } - dtTracer = null; - externalTracer = null; + setDtTracer(null); + setExternalTracer(null); } /** @@ -145,21 +147,26 @@ void reportExternalCall(HttpURLConnection connection, Ops operation, int respons String uri = URISupport.getURI(connection.getURL()); InboundWrapper inboundWrapper = new InboundWrapper(connection); - // This will result in External rollup metrics being generated (e.g. External/all, External/allWeb, External/allOther, External/{HOST}/all) - // Calling reportAsExternal is what causes an HTTP span to be created - externalTracer.reportAsExternal(HttpParameters - .library(LIBRARY) - .uri(URI.create(uri)) - .procedure(operation.label) - .inboundHeaders(inboundWrapper) - .status(responseCode, responseMessage) - .build()); - - // need to call this method to set addedOutboundRequestHeaders in the Tracer - externalTracer.addOutboundRequestHeaders(DummyHeaders.INSTANCE); - GuidSwapper.swap(dtTracer, externalTracer); - dtTracer = null; - externalTracer = null; + TracedMethod externalTracer = getExternalTracer(); + + if (externalTracer != null) { + // This will result in External rollup metrics being generated (e.g. External/all, External/allWeb, External/allOther, External/{HOST}/all) + // Calling reportAsExternal is what causes an HTTP span to be created + externalTracer.reportAsExternal(HttpParameters + .library(LIBRARY) + .uri(URI.create(uri)) + .procedure(operation.label) + .inboundHeaders(inboundWrapper) + .status(responseCode, responseMessage) + .build()); + + // need to call this method to set addedOutboundRequestHeaders in the Tracer + externalTracer.addOutboundRequestHeaders(DummyHeaders.INSTANCE); + GuidSwapper.swap(getDtTracer(), externalTracer); + } + + setDtTracer(null); + setExternalTracer(null); externalReported = true; } } @@ -168,12 +175,28 @@ void reportExternalCall(HttpURLConnection connection, Ops operation, int respons * Checks whether outboundheaders (DT/CAT) were already added and if not, add them to the connection. */ private void addOutboundHeadersIfNotAdded(HttpURLConnection connection) { - if (dtTracer == null) { - dtTracer = AgentBridge.getAgent().getTracedMethod(); - dtTracer.addOutboundRequestHeaders(new OutboundWrapper(connection)); + if (getDtTracer() == null) { + setDtTracer(AgentBridge.getAgent().getTracedMethod()); + getDtTracer().addOutboundRequestHeaders(new OutboundWrapper(connection)); } } + private TracedMethod getDtTracer() { + return dtTracerRef != null ? dtTracerRef.get() : null; + } + + private void setDtTracer(TracedMethod tracedMethod) { + dtTracerRef = tracedMethod != null ? new WeakReference<>(tracedMethod) : null; + } + + private TracedMethod getExternalTracer() { + return externalTracerRef != null ? externalTracerRef.get() : null; + } + + private void setExternalTracer(TracedMethod tracedMethod) { + externalTracerRef = tracedMethod != null ? new WeakReference<>(tracedMethod) : null; + } + public enum Ops { CONNECT("connect"), GET_OUTPUT_STREAM("getOutputStream"), From f9a987c24bb49f037fdff8680b04b32ea12f64a5 Mon Sep 17 00:00:00 2001 From: Oren Ben-Meir Date: Tue, 1 Oct 2024 14:40:34 -0400 Subject: [PATCH 2/4] Fix a possible issue with a weak reference returning null --- .../agent/instrumentation/httpurlconnection/MetricState.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java b/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java index 225a228ef5..a30e38f0b4 100644 --- a/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java +++ b/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java @@ -102,8 +102,10 @@ public void inboundPreamble(boolean isConnected, HttpURLConnection connection, T */ public void inboundPostamble(HttpURLConnection connection, int responseCode, String responseMessage, Ops operation, TracedMethod tracer) { + // So the weak reference to external tracer holds an actual tracer instance (if not null) for the duration of this method. + TracedMethod externalTracer = getExternalTracer(); // make sure that only the method that first invoked inboundPreamble runs this method - if (externalReported || getExternalTracer() != tracer) { + if (externalReported || externalTracer != tracer) { return; } Transaction tx = AgentBridge.getAgent().getTransaction(false); From 6161e5702953a89ad3051f751680dfc3cf036d9f Mon Sep 17 00:00:00 2001 From: Oren Ben-Meir Date: Tue, 1 Oct 2024 14:48:51 -0400 Subject: [PATCH 3/4] Maintain ref of externalTracer by passing to reportExternalCall --- .../httpurlconnection/MetricState.java | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java b/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java index a30e38f0b4..a3dc8002ee 100644 --- a/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java +++ b/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java @@ -110,7 +110,7 @@ public void inboundPostamble(HttpURLConnection connection, int responseCode, Str } Transaction tx = AgentBridge.getAgent().getTransaction(false); if (tx != null) { - reportExternalCall(connection, operation, responseCode, responseMessage); + reportExternalCall(connection, operation, responseCode, responseMessage, externalTracer); } } @@ -144,13 +144,26 @@ public void handleException(TracedMethod tracer, Exception e) { * @param responseMessage response message from HttpURLConnection */ void reportExternalCall(HttpURLConnection connection, Ops operation, int responseCode, String responseMessage) { + reportExternalCall(connection, operation, responseCode, responseMessage, getExternalTracer()); + } + + /** + * Calls the reportAsExternal API. This results in a Span being created for the current TracedMethod/Segment and the Span + * category being set to http which represents a Span that made an external http request. This is required for external + * calls to be properly recorded when they are made to a host that isn't another APM entity. + * + * @param connection HttpURLConnection instance + * @param operation + * @param responseCode response code from HttpURLConnection + * @param responseMessage response message from HttpURLConnection + * @param externalTracer tracer of which the external call will be reported to + */ + void reportExternalCall(HttpURLConnection connection, Ops operation, int responseCode, String responseMessage, TracedMethod externalTracer) { if (connection != null) { // This conversion is necessary as it strips query parameters from the URI String uri = URISupport.getURI(connection.getURL()); InboundWrapper inboundWrapper = new InboundWrapper(connection); - TracedMethod externalTracer = getExternalTracer(); - if (externalTracer != null) { // This will result in External rollup metrics being generated (e.g. External/all, External/allWeb, External/allOther, External/{HOST}/all) // Calling reportAsExternal is what causes an HTTP span to be created From d60721a677ea0349ca6aa85e82475e9919b886a6 Mon Sep 17 00:00:00 2001 From: Oren Ben-Meir Date: Tue, 1 Oct 2024 14:51:45 -0400 Subject: [PATCH 4/4] Make reportExternalCall private when externalTracer passed --- .../nr/agent/instrumentation/httpurlconnection/MetricState.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java b/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java index a3dc8002ee..529d3014b8 100644 --- a/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java +++ b/instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java @@ -158,7 +158,7 @@ void reportExternalCall(HttpURLConnection connection, Ops operation, int respons * @param responseMessage response message from HttpURLConnection * @param externalTracer tracer of which the external call will be reported to */ - void reportExternalCall(HttpURLConnection connection, Ops operation, int responseCode, String responseMessage, TracedMethod externalTracer) { + private void reportExternalCall(HttpURLConnection connection, Ops operation, int responseCode, String responseMessage, TracedMethod externalTracer) { if (connection != null) { // This conversion is necessary as it strips query parameters from the URI String uri = URISupport.getURI(connection.getURL());