Skip to content

Commit

Permalink
Changing HttpUrlConnection instr to not rely on segment/cleanup thread
Browse files Browse the repository at this point in the history
  • Loading branch information
meiao committed Oct 4, 2023
1 parent 2c20813 commit afe7295
Show file tree
Hide file tree
Showing 11 changed files with 539 additions and 512 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public class HttpURLConnectionTest {
static final TransactionDataList transactions = new TransactionDataList();
private static final String GET_OUTPUT_STREAM = "getOutputStream";
private static final String GET_INPUT_STREAM = "getInputStream";
private static final String GET_RESPONSE_CODE = "getResponseCode";
private static final String GET_RESPONSE_MSG = "getResponseMessage";
private static final String URL = "example.com";
private static final String REAL_HOST = "https://example.com";
private static final String FAKE_HOST = "http://www.thishostdoesnotexistbro.com"; // Better hope no one buys this domain...
Expand Down Expand Up @@ -95,7 +97,7 @@ public void outputStreamFirstTest() {
doOutputStreamFirstTest();
String scope = format("OtherTransaction/Custom/{0}/doOutputStreamFirstTest", TEST_CLASS);

verifyMetrics(URL, scope, true, GET_OUTPUT_STREAM);
verifyMetrics(URL, scope, false, GET_OUTPUT_STREAM);
}

@Trace(dispatcher = true)
Expand Down Expand Up @@ -130,7 +132,7 @@ public void connectFirstTest() {
doConnectFirstTest();
String scope = format("OtherTransaction/Custom/{0}/doConnectFirstTest", TEST_CLASS);

verifyMetrics(URL, scope, true, GET_OUTPUT_STREAM);
verifyMetrics(URL, scope, false, GET_OUTPUT_STREAM);
}

@Trace(dispatcher = true)
Expand Down Expand Up @@ -255,7 +257,7 @@ public void testHttpURLConnectionMetrics3() {
run3();
String scope = format("OtherTransaction/Custom/{0}/run3", TEST_CLASS);

verifyMetrics(URL, scope, true, GET_INPUT_STREAM);
verifyMetrics(URL, scope, true, GET_RESPONSE_CODE);
}

@Trace(dispatcher = true)
Expand Down Expand Up @@ -330,7 +332,7 @@ public void testHttpURLConnectionMetrics6() {
run6();
String scope = format("OtherTransaction/Custom/{0}/run6", TEST_CLASS);

verifyMetrics(URL, scope, true, GET_OUTPUT_STREAM);
verifyMetrics(URL, scope, false, GET_OUTPUT_STREAM);
}

@Trace(dispatcher = true)
Expand All @@ -355,7 +357,7 @@ public void testHttpURLConnectionMetrics7() {
run7();
String scope = format("OtherTransaction/Custom/{0}/run7", TEST_CLASS);

verifyMetrics(URL, scope, true, GET_OUTPUT_STREAM);
verifyMetrics(URL, scope, true, GET_RESPONSE_CODE);
}

@Trace(dispatcher = true)
Expand Down Expand Up @@ -383,7 +385,7 @@ public void testHttpURLConnectionMetrics8() {
run8();
String scope = format("OtherTransaction/Custom/{0}/run8", TEST_CLASS);

verifyMetrics(URL, scope, true, GET_OUTPUT_STREAM);
verifyMetrics(URL, scope, true, GET_RESPONSE_CODE);
}

@Trace(dispatcher = true)
Expand All @@ -410,7 +412,7 @@ public void testHttpURLConnectionMetrics9() {
run9();
String scope = format("OtherTransaction/Custom/{0}/run9", TEST_CLASS);

verifyMetrics(URL, scope, true, GET_INPUT_STREAM);
verifyMetrics(URL, scope, true, GET_RESPONSE_CODE);
}

@Trace(dispatcher = true)
Expand All @@ -430,6 +432,31 @@ private void run9() {
}
}

@Test
public void testHttpURLConnectionMetrics10() {
run10();
String scope = format("OtherTransaction/Custom/{0}/run10", TEST_CLASS);

verifyMetrics(URL, scope, true, GET_RESPONSE_MSG);
}

@Trace(dispatcher = true)
private void run10() {
HttpURLConnection connection = null;
try {
// Test 9: getResponseMessage()
connection = getHttpsExampleComConnection();
// GETs URL and reads response message (Network I/O)
System.out.println("Response message: " + connection.getResponseMessage());
} catch (Exception e) {
e.printStackTrace();
} finally {
if (connection != null) {
connection.disconnect();
}
}
}

@Test
public void testUnresolvedHost() throws Exception {
unresolvedHost();
Expand Down Expand Up @@ -475,7 +502,8 @@ private void verifyMetrics(String url, String scope, boolean reportedExternalCal
Set<String> metrics = AgentHelper.getMetrics();
// Scoped external metrics
String httpURLConnectionGetInputStreamMetric = format("External/{0}/HttpURLConnection/getInputStream", url);
String httpURLConnectionGetOutputStreamMetric = format("External/{0}/HttpURLConnection/getOutputStream", url);
String httpURLConnectionGetResponseCodeMetric = format("External/{0}/HttpURLConnection/getResponseCode", url);
String httpURLConnectionGetResponseMessageMetric = format("External/{0}/HttpURLConnection/getResponseMessage", url);
// Unscoped external metrics
String externalHostAllMetric = format("External/{0}/all", url);
String externalAllMetric = "External/all";
Expand All @@ -488,8 +516,7 @@ private void verifyMetrics(String url, String scope, boolean reportedExternalCal
scopedMetricCount = 1;
// One of these scoped metrics should be generated when an external call is reported
Assert.assertTrue(metrics.toString(),
metrics.contains(httpURLConnectionGetOutputStreamMetric) ||
metrics.contains(httpURLConnectionGetInputStreamMetric));
metrics.contains(format("External/{0}/HttpURLConnection/" + methodInExternalMetric, url)));

unscopedMetricCount = 3;
// All three of these unscoped metrics should be generated when an external call is reported
Expand All @@ -498,25 +525,27 @@ private void verifyMetrics(String url, String scope, boolean reportedExternalCal
Assert.assertTrue(metrics.toString(), metrics.contains(externalAllOtherMetric));
} else {
Assert.assertFalse(metrics.toString(),
metrics.contains(httpURLConnectionGetOutputStreamMetric) ||
metrics.contains(httpURLConnectionGetInputStreamMetric));
metrics.contains(format("External/{0}/HttpURLConnection/" + methodInExternalMetric, url)));

Assert.assertFalse(metrics.toString(), metrics.contains(externalHostAllMetric));
Assert.assertFalse(metrics.toString(), metrics.contains(externalAllMetric));
Assert.assertFalse(metrics.toString(), metrics.contains(externalAllOtherMetric));
}

Map<String, Integer> scopedMetricCounts = getMetricCounts(
MetricName.create(httpURLConnectionGetOutputStreamMetric, scope),
MetricName.create(httpURLConnectionGetInputStreamMetric, scope));
MetricName.create(httpURLConnectionGetInputStreamMetric, scope),
MetricName.create(httpURLConnectionGetResponseCodeMetric, scope),
MetricName.create(httpURLConnectionGetResponseMessageMetric, scope)
);

Map<String, Integer> unscopedMetricCounts = getMetricCounts(
MetricName.create(externalHostAllMetric),
MetricName.create(externalAllMetric),
MetricName.create(externalAllOtherMetric));

int actualHttpURLConnectionScopedMetricCount = scopedMetricCounts.get(httpURLConnectionGetOutputStreamMetric) +
scopedMetricCounts.get(httpURLConnectionGetInputStreamMetric);
int actualHttpURLConnectionScopedMetricCount = scopedMetricCounts.get(httpURLConnectionGetInputStreamMetric) +
scopedMetricCounts.get(httpURLConnectionGetResponseCodeMetric) +
scopedMetricCounts.get(httpURLConnectionGetResponseMessageMetric);

int actualHttpURLConnectionUnscopedMetricCount = unscopedMetricCounts.get(externalHostAllMetric) +
unscopedMetricCounts.get(externalAllMetric) +
Expand All @@ -525,15 +554,17 @@ private void verifyMetrics(String url, String scope, boolean reportedExternalCal
assertEquals(scopedMetricCount, actualHttpURLConnectionScopedMetricCount);

if (scopedMetricCount == 0) {
assertEquals(0, (int) scopedMetricCounts.get(httpURLConnectionGetOutputStreamMetric));
assertEquals(0, (int) scopedMetricCounts.get(httpURLConnectionGetInputStreamMetric));
} else {
if (methodInExternalMetric != null) {
if (methodInExternalMetric.equals(GET_INPUT_STREAM)) {
assertEquals(1, (int) scopedMetricCounts.get(httpURLConnectionGetInputStreamMetric));
}
if (methodInExternalMetric.equals(GET_OUTPUT_STREAM)) {
assertEquals(1, (int) scopedMetricCounts.get(httpURLConnectionGetOutputStreamMetric));
if (methodInExternalMetric.equals(GET_RESPONSE_CODE)) {
assertEquals(1, (int) scopedMetricCounts.get(httpURLConnectionGetResponseCodeMetric));
}
if (methodInExternalMetric.equals(GET_RESPONSE_MSG)) {
assertEquals(1, (int) scopedMetricCounts.get(httpURLConnectionGetResponseMessageMetric));
}
}
}
Expand Down
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/getInputStream"));
assertTrue(metrics.toString(), metrics.contains("External/" + HOST + "/HttpURLConnection/getResponseCode"));

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

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

@Trace(dispatcher = true)
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/*
*
* * Copyright 2023 New Relic Corporation. All rights reserved.
* * SPDX-License-Identifier: Apache-2.0
*
*/

package com.nr.agent.instrumentation.httpurlconnection;

import com.newrelic.api.agent.TracedMethod;

import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

/**
* This class is an attempt to make swapping tracers' guids less hacky.
*/
class GuidSwapper {

/**
* Swaps the guids from the provided tracers.
*/
static void swap(TracedMethod tracer1, TracedMethod tracer2) {
if (tracer1 == tracer2 || tracer1 == null || tracer2 == null) {
return;
}
GuidChanger changer1 = GuidSwapper.getGuidChanger(tracer1.getClass());
GuidChanger changer2 = GuidSwapper.getGuidChanger(tracer2.getClass());

if (changer1 != GuidChanger.NULL_GUIDCHANGER && changer2 != GuidChanger.NULL_GUIDCHANGER) {
String guid1 = changer1.getGuid(tracer1);
String guid2 = changer2.getGuid(tracer2);
changer1.setGuid(tracer1, guid2);
changer2.setGuid(tracer2, guid1);
}
}

// this cache should not be big, maybe just a single entry, so there is no mechanism to expire items
private static Map<Class<? extends TracedMethod>, GuidChanger> CACHE = new ConcurrentHashMap<>(8);

/**
* Get a GuidChanger for a given TracedMethod subclass.
* In most, if not all, cases it will be a DefaultTracer.
* But this makes sure that it will be able to read/write the guid regardless of class.
*
*/
private static GuidChanger getGuidChanger(Class<? extends TracedMethod> clazz) {
GuidChanger guidChanger = CACHE.get(clazz);
if (guidChanger != null) {
return guidChanger;
}

Field field = null;
for (Field declaredField : clazz.getDeclaredFields()) {
if ("guid".equals(declaredField.getName())) {
// make sure the field is not final
if ((declaredField.getModifiers() & Modifier.FINAL) == 0 ) {
field = declaredField;
field.setAccessible(true);
}
break;
}
}
guidChanger = GuidChanger.forField(field);
CACHE.put(clazz, guidChanger);
return guidChanger;
}

private static class GuidChanger {

// Dummy instance for classes that do not have a guid
static final GuidChanger NULL_GUIDCHANGER = new GuidChanger(null);

static GuidChanger forField(Field field) {
return field == null ? NULL_GUIDCHANGER : new GuidChanger(field);
}

private final Field declaredField;

private GuidChanger(Field declaredField) {
this.declaredField = declaredField;
}

public String getGuid(Object target) {
try {
return declaredField.get(target).toString();
} catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
}

public void setGuid(Object target, String guid) {
try {
declaredField.set(target, guid);
} catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -20,39 +20,11 @@
* See README for config examples.
*/
public class HttpURLConnectionConfig {
private static final String configPrefix = "class_transformer.com.newrelic.instrumentation.httpurlconnection.segment_cleanup_task.";
/*
* The following tests do a Thread.sleep to account for this delay. If this value is changed then the tests will also need to be updated.
* functional_test/src/test/java/com/newrelic/agent/instrumentation/pointcuts/net/HttpURLConnectionTest
* instrumentation/httpurlconnection/src/test/java/com/nr/agent/instrumentation/httpurlconnection/MetricStateConnectTest
*/
private static final int DEFAULT_TASK_DELAY_MS = 5_000;
private static final int DEFAULT_THREAD_POOL_SIZE = 5;
private static final boolean DEFAULT_DISTRIBUTED_TRACING_ENABLED = true;

private HttpURLConnectionConfig() {
}

public static int getThreadPoolSize() {
int threadPoolSize = DEFAULT_THREAD_POOL_SIZE;
try {
threadPoolSize = NewRelic.getAgent().getConfig().getValue(configPrefix + "thread_pool_size", DEFAULT_THREAD_POOL_SIZE);
} catch (Exception e) {
AgentBridge.getAgent().getLogger().log(Level.FINEST, "HttpURLConnection - using default thread_pool_size: " + threadPoolSize);
}
return threadPoolSize;
}

public static int getDelayMs() {
int delayMs = DEFAULT_TASK_DELAY_MS;
try {
delayMs = NewRelic.getAgent().getConfig().getValue(configPrefix + "delay_ms", DEFAULT_TASK_DELAY_MS);
} catch (Exception e) {
AgentBridge.getAgent().getLogger().log(Level.FINEST, "HttpURLConnection - using default task delay_ms: " + delayMs);
}
return delayMs;
}

public static boolean distributedTracingEnabled() {
boolean dtEnabled = DEFAULT_DISTRIBUTED_TRACING_ENABLED;
try {
Expand Down
Loading

0 comments on commit afe7295

Please sign in to comment.