Skip to content

Commit ab5fa74

Browse files
buongarzonibuongarzoni
committed
fix(java-agent): type Apache advice parameters as Object
1 parent dd89925 commit ab5fa74

4 files changed

Lines changed: 217 additions & 51 deletions

File tree

‎rollbar-java-agent/src/main/java/com/rollbar/agent/instrumentation/ApacheHttpClient4Instrumentation.java‎

Lines changed: 76 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,6 @@
55
import net.bytebuddy.agent.builder.AgentBuilder;
66
import net.bytebuddy.asm.Advice;
77
import net.bytebuddy.matcher.ElementMatchers;
8-
import org.apache.http.HttpHost;
9-
import org.apache.http.HttpRequest;
10-
import org.apache.http.HttpResponse;
118

129
/**
1310
* Installs ByteBuddy advice on Apache HttpClient 4.x to capture network errors.
@@ -63,8 +60,78 @@ public static void installIfAvailable(AgentBuilder builder, Instrumentation inst
6360
}
6461

6562
/**
66-
* Apache HC 4.x runs in the application classloader, so we can reference Rollbar classes
67-
* directly without the TCCL reflection bridge.
63+
* Records a 4xx/5xx HC 4.x response as telemetry; other statuses are ignored. Called by
64+
* {@link DoExecuteAdvice}, which cannot name {@code org.apache.http} types itself.
65+
*
66+
* <p>Members are read through the public {@code org.apache.http} interfaces rather than the
67+
* concrete class of each object, because HC 4.x hands back package-private implementations —
68+
* {@code doExecute} returns {@code org.apache.http.impl.execchain.HttpResponseProxy} — and a
69+
* {@link java.lang.reflect.Method} looked up on such a class cannot be invoked. The reflection
70+
* runs once per request, which is immaterial next to the HTTP call it describes.
71+
*
72+
* @param target the request target the client dispatched to, or null
73+
* @param request the executed request
74+
* @param response the response returned by {@code doExecute}
75+
*/
76+
public static void recordResponse(Object target, Object request, Object response) {
77+
try {
78+
Object statusLine = invokeVia("org.apache.http.HttpResponse", response, "getStatusLine");
79+
if (statusLine == null) {
80+
return;
81+
}
82+
int statusCode =
83+
(Integer) invokeVia("org.apache.http.StatusLine", statusLine, "getStatusCode");
84+
if (statusCode < 400) {
85+
return;
86+
}
87+
Object requestLine = invokeVia("org.apache.http.HttpRequest", request, "getRequestLine");
88+
if (requestLine == null) {
89+
return;
90+
}
91+
String method = (String) invokeVia("org.apache.http.RequestLine", requestLine, "getMethod");
92+
String requestUri = (String) invokeVia("org.apache.http.RequestLine", requestLine, "getUri");
93+
// The host-based overloads carry the target separately from a request whose URI may be
94+
// just a path, so rejoin the two rather than reading the request URI alone.
95+
String base = target != null
96+
? (String) invokeVia("org.apache.http.HttpHost", target, "toURI") : null;
97+
NetworkEventBridge.recordNetworkEvent(
98+
response,
99+
method,
100+
NetworkEventBridge.composeUrl(base, requestUri),
101+
String.valueOf(statusCode)
102+
);
103+
} catch (Throwable ignored) {
104+
// Telemetry must never disrupt the instrumented request
105+
}
106+
}
107+
108+
/**
109+
* Invokes {@code methodName} on {@code receiver} through the named public API type, resolved from
110+
* the receiver's own classloader — the one that loaded HC 4.x, which the agent's classloader may
111+
* not be able to see.
112+
*/
113+
private static Object invokeVia(String apiTypeName, Object receiver, String methodName)
114+
throws ReflectiveOperationException {
115+
ClassLoader classLoader = receiver.getClass().getClassLoader();
116+
Class<?> apiType = Class.forName(apiTypeName, false,
117+
classLoader != null ? classLoader : ClassLoader.getSystemClassLoader());
118+
return apiType.getMethod(methodName).invoke(receiver);
119+
}
120+
121+
/**
122+
* Apache HC 4.x runs in the application classloader, so the advice body can reference Rollbar
123+
* classes directly without the TCCL reflection bridge.
124+
*
125+
* <p>The advice signature, however, must not name {@code org.apache.http} types.
126+
* {@code Advice.to(DoExecuteAdvice.class)} resolves this method's parameter and return types via
127+
* {@link Class#getDeclaredMethods()}, in the classloader that loaded the advice class — the
128+
* agent's, which under {@code -javaagent} is the system classloader. Wherever HC 4.x is loaded by
129+
* a child classloader the agent cannot see (Spring Boot executable jars, per-WAR container
130+
* classloaders, OSGi bundles), that lookup throws {@link NoClassDefFoundError} inside the
131+
* transformer; the AgentBuilder reports it and moves on, and HC 4.x is silently never
132+
* instrumented. So everything the advice touches is typed {@link Object} and read reflectively in
133+
* {@link ApacheHttpClient4Instrumentation#recordResponse}, which runs after the weave and can
134+
* resolve those types from the client's own classloader.
68135
*
69136
* <p>String concatenation in the advice body must use {@link String#concat} or
70137
* {@link StringBuilder} rather than the {@code +} operator. Apache HC 4.x jars are compiled at
@@ -84,9 +151,9 @@ public static class DoExecuteAdvice {
84151
*/
85152
@Advice.OnMethodExit(onThrowable = Throwable.class)
86153
public static void onExit(
87-
@Advice.Argument(0) HttpHost target,
88-
@Advice.Argument(1) HttpRequest request,
89-
@Advice.Return HttpResponse response,
154+
@Advice.Argument(0) Object target,
155+
@Advice.Argument(1) Object request,
156+
@Advice.Return Object response,
90157
@Advice.Thrown Throwable thrown
91158
) {
92159
try {
@@ -100,20 +167,7 @@ public static void onExit(
100167
}
101168

102169
if (response != null && request != null) {
103-
int statusCode = response.getStatusLine().getStatusCode();
104-
if (statusCode >= 400) {
105-
// The host-based overloads carry the target separately from a request whose URI may be
106-
// just a path, so rejoin the two rather than reading the request URI alone.
107-
String base = target != null ? target.toURI() : null;
108-
String requestUri = request.getRequestLine() != null
109-
? request.getRequestLine().getUri() : null;
110-
NetworkEventBridge.recordNetworkEvent(
111-
response,
112-
request.getRequestLine().getMethod(),
113-
NetworkEventBridge.composeUrl(base, requestUri),
114-
String.valueOf(statusCode)
115-
);
116-
}
170+
ApacheHttpClient4Instrumentation.recordResponse(target, request, response);
117171
}
118172
} catch (Throwable ignored) {
119173
// Advice must never throw

‎rollbar-java-agent/src/main/java/com/rollbar/agent/instrumentation/ApacheHttpClient5Instrumentation.java‎

Lines changed: 79 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,11 @@
22

33
import com.rollbar.agent.NetworkEventBridge;
44
import java.lang.instrument.Instrumentation;
5-
import java.net.URI;
6-
import java.net.URISyntaxException;
5+
import java.lang.reflect.InvocationTargetException;
76

87
import net.bytebuddy.agent.builder.AgentBuilder;
98
import net.bytebuddy.asm.Advice;
109
import net.bytebuddy.matcher.ElementMatchers;
11-
import org.apache.hc.core5.http.ClassicHttpRequest;
12-
import org.apache.hc.core5.http.ClassicHttpResponse;
13-
import org.apache.hc.core5.http.HttpHost;
1410

1511
/**
1612
* Installs ByteBuddy advice on Apache HttpClient 5.x to capture network errors.
@@ -68,8 +64,80 @@ public static void installIfAvailable(AgentBuilder builder, Instrumentation inst
6864
}
6965

7066
/**
71-
* Apache HC 5.x runs in the application classloader, so we can reference Rollbar classes
72-
* directly without the TCCL reflection bridge.
67+
* Records a 4xx/5xx HC 5.x response as telemetry; other statuses are ignored. Called by
68+
* {@link DoExecuteAdvice}, which cannot name {@code org.apache.hc} types itself.
69+
*
70+
* <p>Members are read through the public {@code org.apache.hc.core5.http} interfaces rather than
71+
* the concrete class of each object, because HC 5.x hands back package-private implementations —
72+
* {@code doExecute} returns the adapter {@code CloseableHttpResponse.adapt} produces — and a
73+
* {@link java.lang.reflect.Method} looked up on such a class cannot be invoked. The reflection
74+
* runs once per request, which is immaterial next to the HTTP call it describes.
75+
*
76+
* @param target the request target the client dispatched to, or null
77+
* @param request the executed request
78+
* @param response the response returned by {@code doExecute}
79+
*/
80+
public static void recordResponse(Object target, Object request, Object response) {
81+
try {
82+
int statusCode =
83+
(Integer) invokeVia("org.apache.hc.core5.http.HttpResponse", response, "getCode");
84+
if (statusCode < 400) {
85+
return;
86+
}
87+
String requestUri;
88+
try {
89+
Object uri = invokeVia("org.apache.hc.core5.http.HttpRequest", request, "getUri");
90+
requestUri = uri != null ? uri.toString() : null;
91+
} catch (InvocationTargetException ignored) {
92+
// getUri() throws URISyntaxException for a request URI HC could not assemble; the raw
93+
// request URI is still worth recording.
94+
requestUri =
95+
(String) invokeVia("org.apache.hc.core5.http.HttpRequest", request, "getRequestUri");
96+
}
97+
String method =
98+
(String) invokeVia("org.apache.hc.core5.http.HttpRequest", request, "getMethod");
99+
// The host-based overloads carry the target separately from a request whose URI may be
100+
// just a path, so rejoin the two rather than reading the request URI alone.
101+
String base = target != null
102+
? (String) invokeVia("org.apache.hc.core5.http.HttpHost", target, "toURI") : null;
103+
NetworkEventBridge.recordNetworkEvent(
104+
response,
105+
method,
106+
NetworkEventBridge.composeUrl(base, requestUri),
107+
String.valueOf(statusCode)
108+
);
109+
} catch (Throwable ignored) {
110+
// Telemetry must never disrupt the instrumented request
111+
}
112+
}
113+
114+
/**
115+
* Invokes {@code methodName} on {@code receiver} through the named public API type, resolved from
116+
* the receiver's own classloader — the one that loaded HC 5.x, which the agent's classloader may
117+
* not be able to see.
118+
*/
119+
private static Object invokeVia(String apiTypeName, Object receiver, String methodName)
120+
throws ReflectiveOperationException {
121+
ClassLoader classLoader = receiver.getClass().getClassLoader();
122+
Class<?> apiType = Class.forName(apiTypeName, false,
123+
classLoader != null ? classLoader : ClassLoader.getSystemClassLoader());
124+
return apiType.getMethod(methodName).invoke(receiver);
125+
}
126+
127+
/**
128+
* Apache HC 5.x runs in the application classloader, so the advice body can reference Rollbar
129+
* classes directly without the TCCL reflection bridge.
130+
*
131+
* <p>The advice signature, however, must not name {@code org.apache.hc} types.
132+
* {@code Advice.to(DoExecuteAdvice.class)} resolves this method's parameter and return types via
133+
* {@link Class#getDeclaredMethods()}, in the classloader that loaded the advice class — the
134+
* agent's, which under {@code -javaagent} is the system classloader. Wherever HC 5.x is loaded by
135+
* a child classloader the agent cannot see (Spring Boot executable jars, per-WAR container
136+
* classloaders, OSGi bundles), that lookup throws {@link NoClassDefFoundError} inside the
137+
* transformer; the AgentBuilder reports it and moves on, and HC 5.x is silently never
138+
* instrumented. So everything the advice touches is typed {@link Object} and read reflectively in
139+
* {@link ApacheHttpClient5Instrumentation#recordResponse}, which runs after the weave and can
140+
* resolve those types from the client's own classloader.
73141
*/
74142
public static class DoExecuteAdvice {
75143

@@ -83,9 +151,9 @@ public static class DoExecuteAdvice {
83151
*/
84152
@Advice.OnMethodExit(onThrowable = Throwable.class)
85153
public static void onExit(
86-
@Advice.Argument(0) HttpHost target,
87-
@Advice.Argument(1) ClassicHttpRequest request,
88-
@Advice.Return ClassicHttpResponse response,
154+
@Advice.Argument(0) Object target,
155+
@Advice.Argument(1) Object request,
156+
@Advice.Return Object response,
89157
@Advice.Thrown Throwable thrown
90158
) {
91159
try {
@@ -99,25 +167,7 @@ public static void onExit(
99167
}
100168

101169
if (response != null && request != null) {
102-
int statusCode = response.getCode();
103-
if (statusCode >= 400) {
104-
String requestUri;
105-
try {
106-
URI uri = request.getUri();
107-
requestUri = uri != null ? uri.toString() : null;
108-
} catch (URISyntaxException ignored) {
109-
requestUri = request.getRequestUri();
110-
}
111-
// The host-based overloads carry the target separately from a request whose URI may be
112-
// just a path, so rejoin the two rather than reading the request URI alone.
113-
String base = target != null ? target.toURI() : null;
114-
NetworkEventBridge.recordNetworkEvent(
115-
response,
116-
request.getMethod(),
117-
NetworkEventBridge.composeUrl(base, requestUri),
118-
String.valueOf(statusCode)
119-
);
120-
}
170+
ApacheHttpClient5Instrumentation.recordResponse(target, request, response);
121171
}
122172
} catch (Throwable ignored) {
123173
// Advice must never throw

‎rollbar-java-agent/src/test/java/com/rollbar/agent/instrumentation/ApacheHttpClient4InstrumentationTest.java‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import org.junit.jupiter.api.BeforeEach;
1818
import org.junit.jupiter.api.Test;
1919

20+
import java.lang.reflect.Method;
2021
import java.util.List;
2122
import java.util.Map;
2223

@@ -142,4 +143,34 @@ public void urlSanitization_stripsQuery() throws Exception {
142143
assertTrue(url.contains("/path"));
143144
assertFalse(url.contains("secret"));
144145
}
146+
147+
/**
148+
* ByteBuddy resolves an advice method's parameter and return types in the classloader that loaded
149+
* the advice class — the agent's. Naming an {@code org.apache.http} type here throws
150+
* NoClassDefFoundError at weave time wherever HC 4.x lives in a classloader the agent cannot see
151+
* (Spring Boot executable jars, per-WAR container classloaders, OSGi), silently disabling this
152+
* instrumentation. A flat test classpath cannot reproduce that, so pin the signature instead.
153+
*/
154+
@Test
155+
public void adviceSignature_namesNoApacheTypes() {
156+
for (Method method
157+
: ApacheHttpClient4Instrumentation.DoExecuteAdvice.class.getDeclaredMethods()) {
158+
if (method.isSynthetic()) {
159+
continue; // e.g. JaCoCo's $jacocoInit(), which ByteBuddy never resolves as advice
160+
}
161+
for (Class<?> parameterType : method.getParameterTypes()) {
162+
assertTrue(isAgentVisible(parameterType),
163+
"advice parameter type must be resolvable from the agent's classloader: "
164+
+ parameterType.getName());
165+
}
166+
assertTrue(isAgentVisible(method.getReturnType()),
167+
"advice return type must be resolvable from the agent's classloader: "
168+
+ method.getReturnType().getName());
169+
}
170+
}
171+
172+
private static boolean isAgentVisible(Class<?> type) {
173+
String name = type.getName();
174+
return type.isPrimitive() || name.startsWith("java.") || name.startsWith("com.rollbar.");
175+
}
145176
}

‎rollbar-java-agent/src/test/java/com/rollbar/agent/instrumentation/ApacheHttpClient5InstrumentationTest.java‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.junit.jupiter.api.BeforeEach;
1717
import org.junit.jupiter.api.Test;
1818

19+
import java.lang.reflect.Method;
1920
import java.util.List;
2021
import java.util.Map;
2122

@@ -162,4 +163,34 @@ public void urlSanitization_stripsQuery() throws Exception {
162163
assertTrue(url.contains("/path"), "URL should include path: " + url);
163164
assertFalse(url.contains("secret"), "URL should not contain query params: " + url);
164165
}
166+
167+
/**
168+
* ByteBuddy resolves an advice method's parameter and return types in the classloader that loaded
169+
* the advice class — the agent's. Naming an {@code org.apache.hc} type here throws
170+
* NoClassDefFoundError at weave time wherever HC 5.x lives in a classloader the agent cannot see
171+
* (Spring Boot executable jars, per-WAR container classloaders, OSGi), silently disabling this
172+
* instrumentation. A flat test classpath cannot reproduce that, so pin the signature instead.
173+
*/
174+
@Test
175+
public void adviceSignature_namesNoApacheTypes() {
176+
for (Method method
177+
: ApacheHttpClient5Instrumentation.DoExecuteAdvice.class.getDeclaredMethods()) {
178+
if (method.isSynthetic()) {
179+
continue; // e.g. JaCoCo's $jacocoInit(), which ByteBuddy never resolves as advice
180+
}
181+
for (Class<?> parameterType : method.getParameterTypes()) {
182+
assertTrue(isAgentVisible(parameterType),
183+
"advice parameter type must be resolvable from the agent's classloader: "
184+
+ parameterType.getName());
185+
}
186+
assertTrue(isAgentVisible(method.getReturnType()),
187+
"advice return type must be resolvable from the agent's classloader: "
188+
+ method.getReturnType().getName());
189+
}
190+
}
191+
192+
private static boolean isAgentVisible(Class<?> type) {
193+
String name = type.getName();
194+
return type.isPrimitive() || name.startsWith("java.") || name.startsWith("com.rollbar.");
195+
}
165196
}

0 commit comments

Comments
 (0)