Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix httpurlconnection inst #885

Merged
merged 10 commits into from
Jul 5, 2022
Merged

Fix httpurlconnection inst #885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3acebd3
Update HttpURLConnection to use reportAsExternal API
jasonjkeller May 17, 2022
9bd6b8d
Merge branch 'main' of github.com:newrelic/newrelic-java-agent into f…
jasonjkeller Jun 1, 2022
f512aa0
Update httpurlconnection instrumentation to use reportAsExternal API
jasonjkeller Jun 16, 2022
457af57
Merge branch 'main' of github.com:newrelic/newrelic-java-agent into f…
jasonjkeller Jun 16, 2022
a78c5b3
Add qualified import
jasonjkeller Jun 16, 2022
9dc22bc
Fix HttpURLConnectionTest
jasonjkeller Jun 17, 2022
64401b9
Fix HttpCommonsTest
jasonjkeller Jun 17, 2022
877847a
Add outbound CAT headers
jasonjkeller Jun 17, 2022
fbdb11d
Correctly add CAT/Distributed tracing headers to outbound request
jasonjkeller Jun 21, 2022
81c521b
Clarify Javadoc comment on deprecated API
jasonjkeller Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions agent-bridge/src/main/java/com/newrelic/agent/bridge/external/ExternalMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public static void makeExternalComponentMetric(TracedMethod method, String host,
// transaction segment name always contains operations; metric name may or may not
String operationsPath = fixOperations(operations);

if(operationsPath == null) {
if (operationsPath == null) {
String metricName = MessageFormat.format(METRIC_NAME, host, library);
method.setMetricNameFormatInfo(metricName, metricName, uri);
} else {
Expand Down Expand Up @@ -89,7 +89,7 @@ public static void makeExternalComponentTrace(boolean isWebTransaction, TracedMe

makeExternalComponentMetric(method, hostName, library, includeOperationInMetric, uri, operations);

if(UNKNOWN_HOST.equals(hostName)) {
if (UNKNOWN_HOST.equals(hostName)) {
return; // NR doesn't add rollup metrics for "UnknownHost"
}

Expand Down
101 changes: 63 additions & 38 deletions ...src/test/java/com/newrelic/agent/instrumentation/pointcuts/net/HttpURLConnectionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,16 @@

package com.newrelic.agent.instrumentation.pointcuts.net;

import static java.text.MessageFormat.format;
import static org.junit.Assert.assertEquals;
import com.google.common.collect.Sets;
import com.newrelic.agent.AgentHelper;
import com.newrelic.agent.TransactionDataList;
import com.newrelic.agent.metric.MetricName;
import com.newrelic.agent.service.ServiceFactory;
import com.newrelic.api.agent.Trace;
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;

import java.io.BufferedReader;
import java.io.BufferedWriter;
Expand All @@ -23,16 +31,8 @@
import java.util.Map;
import java.util.Set;

import com.google.common.collect.Sets;
import com.newrelic.agent.AgentHelper;
import com.newrelic.agent.TransactionDataList;
import com.newrelic.agent.metric.MetricName;
import com.newrelic.agent.service.ServiceFactory;
import com.newrelic.api.agent.Trace;
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
import static java.text.MessageFormat.format;
import static org.junit.Assert.assertEquals;

public class HttpURLConnectionTest {

Expand Down Expand Up @@ -489,43 +489,68 @@ private void verifyMetrics(String url, String scope, int scopedHttpUrlCount, int
int scopedNetworkIOCount, int unscopedNetworkIOCount) {
Set<String> metrics = AgentHelper.getMetrics();

String httpURLConnectionMetric = "External/{0}/HttpURLConnection";
String httpURLConnectionGetInputStreamMetric = "External/{0}/HttpURLConnection/getInputStream";
String httpURLConnectionGetResponseCodeMetric = "External/{0}/HttpURLConnection/getResponseCode";
String externalHostAllMetric = "External/{0}/all";
String externalAllMetric = "External/all";
String externalAllOtherMetric = "External/allOther";

if (scopedHttpUrlCount > 0 || unscopedHttpUrlCount > 0) {
Assert.assertTrue(metrics.toString(), metrics.contains(format("External/{0}/HttpURLConnection", url)));
Assert.assertTrue(metrics.toString(),
metrics.contains(format(httpURLConnectionMetric, url)) ||
metrics.contains(format(httpURLConnectionGetInputStreamMetric, url)) ||
metrics.contains(format(httpURLConnectionGetResponseCodeMetric, url)));
} else {
Assert.assertFalse(metrics.toString(), metrics.contains(format("External/{0}/HttpURLConnection", url)));
Assert.assertFalse(metrics.toString(),
metrics.contains(format(httpURLConnectionMetric, url)) ||
metrics.contains(format(httpURLConnectionGetInputStreamMetric, url)) ||
metrics.contains(format(httpURLConnectionGetResponseCodeMetric, url)));
}

if (scopedNetworkIOCount > 0 || unscopedNetworkIOCount > 0) {
Assert.assertTrue(metrics.toString(), metrics.contains(format("External/{0}/all", url)));
Assert.assertTrue(metrics.toString(), metrics.contains("External/all"));
Assert.assertTrue(metrics.toString(), metrics.contains("External/allOther"));
Assert.assertTrue(metrics.toString(), metrics.contains(format(externalHostAllMetric, url)));
Assert.assertTrue(metrics.toString(), metrics.contains(externalAllMetric));
Assert.assertTrue(metrics.toString(), metrics.contains(externalAllOtherMetric));
} else {
Assert.assertFalse(metrics.toString(), metrics.contains(format("External/{0}/all", url)));
Assert.assertFalse(metrics.toString(), metrics.contains("External/all"));
Assert.assertFalse(metrics.toString(), metrics.contains("External/allOther"));
Assert.assertFalse(metrics.toString(), metrics.contains(format(externalHostAllMetric, url)));
Assert.assertFalse(metrics.toString(), metrics.contains(externalAllMetric));
Assert.assertFalse(metrics.toString(), metrics.contains(externalAllOtherMetric));
}

Map<String, Integer> scopedMetrics = getMetricCounts(
MetricName.create(format("External/{0}/HttpURLConnection", url), scope),
MetricName.create(format("External/{0}/all", url), scope),
MetricName.create("External/all", scope),
MetricName.create("External/allOther", scope));
MetricName.create(format(httpURLConnectionMetric, url), scope),
MetricName.create(format(httpURLConnectionGetInputStreamMetric, url), scope),
MetricName.create(format(httpURLConnectionGetResponseCodeMetric, url), scope),
MetricName.create(format(externalHostAllMetric, url), scope),
MetricName.create(externalAllMetric, scope),
MetricName.create(externalAllOtherMetric, scope));

Map<String, Integer> unscopedMetrics = getMetricCounts(
MetricName.create(format("External/{0}/HttpURLConnection", url)),
MetricName.create(format("External/{0}/all", url)),
MetricName.create("External/all"),
MetricName.create("External/allOther"));

assertEquals(scopedHttpUrlCount, (int) scopedMetrics.get(format("External/{0}/HttpURLConnection", url)));
assertEquals(scopedNetworkIOCount, (int) scopedMetrics.get(format("External/{0}/all", url)));
assertEquals(scopedNetworkIOCount, (int) scopedMetrics.get("External/all"));
assertEquals(scopedNetworkIOCount, (int) scopedMetrics.get("External/allOther"));

assertEquals(unscopedHttpUrlCount, (int) unscopedMetrics.get(format("External/{0}/HttpURLConnection", url)));
assertEquals(unscopedNetworkIOCount, (int) unscopedMetrics.get(format("External/{0}/all", url)));
assertEquals(unscopedNetworkIOCount, (int) unscopedMetrics.get("External/all"));
assertEquals(unscopedNetworkIOCount, (int) unscopedMetrics.get("External/allOther"));
MetricName.create(format(httpURLConnectionMetric, url)),
MetricName.create(format(httpURLConnectionGetInputStreamMetric, url)),
MetricName.create(format(httpURLConnectionGetResponseCodeMetric, url)),
MetricName.create(format(externalHostAllMetric, url)),
MetricName.create(externalAllMetric),
MetricName.create(externalAllOtherMetric));

int actualHttpURLConnectionScopedMetricCount = scopedMetrics.get(format(httpURLConnectionMetric, url)) +
scopedMetrics.get(format(httpURLConnectionGetInputStreamMetric, url)) +
scopedMetrics.get(format(httpURLConnectionGetResponseCodeMetric, url));

int actualHttpURLConnectionUnscopedMetricCount = unscopedMetrics.get(format(httpURLConnectionMetric, url)) +
unscopedMetrics.get(format(httpURLConnectionGetInputStreamMetric, url)) +
unscopedMetrics.get(format(httpURLConnectionGetResponseCodeMetric, url));

assertEquals(scopedHttpUrlCount, actualHttpURLConnectionScopedMetricCount);
assertEquals(scopedNetworkIOCount, (int) scopedMetrics.get(format(externalHostAllMetric, url)));
assertEquals(scopedNetworkIOCount, (int) scopedMetrics.get(externalAllMetric));
assertEquals(scopedNetworkIOCount, (int) scopedMetrics.get(externalAllOtherMetric));

assertEquals(unscopedHttpUrlCount, actualHttpURLConnectionUnscopedMetricCount);
assertEquals(unscopedNetworkIOCount, (int) unscopedMetrics.get(format(externalHostAllMetric, url)));
assertEquals(unscopedNetworkIOCount, (int) unscopedMetrics.get(externalAllMetric));
assertEquals(unscopedNetworkIOCount, (int) unscopedMetrics.get(externalAllOtherMetric));
}

private Map<String, Integer> getMetricCounts(MetricName... responseTimeMetricNames) {
Expand Down
6 changes: 3 additions & 3 deletions functional_test/src/test/java/test/newrelic/test/agent/HttpCommonsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,13 @@ public void httpURLConnection() throws Exception {
httpURLConnectionTx();

Set<String> metrics = AgentHelper.getMetrics();
assertTrue(metrics.toString(), metrics.contains("External/" + HOST + "/HttpURLConnection"));
assertTrue(metrics.toString(), metrics.contains("External/" + HOST + "/HttpURLConnection/getResponseCode"));

Map<String, Integer> metricCounts = getMetricCounts(
MetricName.create("External/" + HOST + "/HttpURLConnection",
MetricName.create("External/" + HOST + "/HttpURLConnection/getResponseCode",
"OtherTransaction/Custom/test.newrelic.test.agent.HttpCommonsTest/httpURLConnectionTx"));

assertEquals(1, (int) metricCounts.get("External/" + HOST + "/HttpURLConnection"));
assertEquals(1, (int) metricCounts.get("External/" + HOST + "/HttpURLConnection/getResponseCode"));
}

@Trace(dispatcher = true)
Expand Down
45 changes: 34 additions & 11 deletions ...lconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@
import com.newrelic.agent.bridge.Transaction;
import com.newrelic.agent.bridge.external.ExternalMetrics;
import com.newrelic.agent.bridge.external.URISupport;
import com.newrelic.api.agent.HttpParameters;

import java.net.HttpURLConnection;
import java.net.URI;

public class MetricState {

private static final String LIBRARY = "HttpURLConnection";
private boolean metricsRecorded;
private boolean recordedANetworkCall;

Expand All @@ -25,8 +27,9 @@ public void nonNetworkPreamble(boolean isConnected, HttpURLConnection connection
Transaction tx = AgentBridge.getAgent().getTransaction(false);
if (!isConnected && method.isMetricProducer() && tx != null) {
// This method doesn't have any network I/O so we are explicitly not recording external rollup metrics
makeMetric(connection, method, operation);
tx.getCrossProcessState().processOutboundRequestHeaders(new OutboundWrapper(connection), method);
makeNonRollupExternalMetric(connection, method, operation);
// Add CAT/Distributed tracing headers to this outbound request
method.addOutboundRequestHeaders(new OutboundWrapper(connection));
}
}

Expand All @@ -35,11 +38,12 @@ public void getInputStreamPreamble(boolean isConnected, HttpURLConnection connec
if (method.isMetricProducer() && tx != null) {
if (!recordedANetworkCall) {
this.recordedANetworkCall = true;
makeMetric(connection, method, "getInputStream");
makeNonRollupExternalMetric(connection, method, "getInputStream");
}

if (!isConnected) {
tx.getCrossProcessState().processOutboundRequestHeaders(new OutboundWrapper(connection), method);
// Add CAT/Distributed tracing headers to this outbound request
method.addOutboundRequestHeaders(new OutboundWrapper(connection));
}
}
}
Expand All @@ -48,27 +52,46 @@ public void getResponseCodePreamble(HttpURLConnection connection, TracedMethod m
Transaction tx = AgentBridge.getAgent().getTransaction(false);
if (method.isMetricProducer() && tx != null && !recordedANetworkCall) {
this.recordedANetworkCall = true;
makeMetric(connection, method, "getResponseCode");
makeNonRollupExternalMetric(connection, method, "getResponseCode");
}
}

public void getInboundPostamble(HttpURLConnection connection, TracedMethod method) {
public void getInboundPostamble(HttpURLConnection connection, int responseCode, String responseMessage, String requestMethod, TracedMethod method) {
Transaction tx = AgentBridge.getAgent().getTransaction(false);
if (method.isMetricProducer() && !metricsRecorded && tx != null) {
this.metricsRecorded = true;
// This conversion is necessary as it strips query parameters from the URI
String uri = URISupport.getURI(connection.getURL());
InboundWrapper inboundWrapper = new InboundWrapper(connection);
tx.getCrossProcessState()
.processInboundResponseHeaders(inboundWrapper, method, connection.getURL().getHost(), uri, true);

// Add CAT/Distributed tracing headers to this outbound request
method.addOutboundRequestHeaders(new OutboundWrapper(connection));

// This will result in External rollup metrics being generated
method.reportAsExternal(HttpParameters
.library(LIBRARY)
.uri(URI.create(uri))
.procedure(requestMethod)
.inboundHeaders(inboundWrapper)
.status(responseCode, responseMessage)
.build());
}
}

private void makeMetric(HttpURLConnection connection, TracedMethod method, String operation) {
/**
* Sets external metric name (i.e. External/{HOST}/HttpURLConnection).
* This does not create rollup metrics such as External/all, External/allWeb, External/allOther, External/{HOST}/all
*
* @param connection HttpURLConnection instance
* @param method TracedMethod instance
* @param operation String representation of operation
*/
private void makeNonRollupExternalMetric(HttpURLConnection connection, TracedMethod method, String operation) {
String uri = URISupport.getURI(connection.getURL());
ExternalMetrics.makeExternalComponentMetric(
method,
connection.getURL().getHost(),
"HttpURLConnection",
LIBRARY,
false,
uri,
operation);
Expand Down
8 changes: 2 additions & 6 deletions instrumentation/httpurlconnection/src/main/java/java/net/HttpURLConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,12 @@ protected HttpURLConnection(URL url) {
@Trace(leaf = true)
public void connect() throws IOException {
lazyGetMetricState().nonNetworkPreamble(connected, this, "connect");

Weaver.callOriginal();
}

@Trace(leaf = true)
public synchronized OutputStream getOutputStream() throws IOException {
lazyGetMetricState().nonNetworkPreamble(connected, this, "getOutputStream");

return Weaver.callOriginal();
}

Expand All @@ -71,8 +69,7 @@ public synchronized InputStream getInputStream() throws IOException {
throw e;
}

metricState.getInboundPostamble(this, method);

metricState.getInboundPostamble(this, 0, null, "getInputStream", method);
return inputStream;
}

Expand All @@ -94,8 +91,7 @@ public int getResponseCode() throws Exception {
throw e;
}

metricState.getInboundPostamble(this, method);

metricState.getInboundPostamble(this, responseCodeValue, null, "getResponseCode", method);
return responseCodeValue;
}

Expand Down
Loading