[Feat] [SDK-399] Add java agent for network telemetry events - #374
buongarzoni wants to merge 44 commits into
Conversation
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
An organization admin can view or raise the cap at claude.ai/admin-settings/claude-code. The cap resets at the start of the next billing period.
Once the cap resets or is raised, comment @claude review on this pull request to trigger a review.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad0b2acf09
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…r<Long> in AgentTelemetryStore
…C5 instrumentation
|
@claude review |
brianr
left a comment
brianr
left a comment
There was a problem hiding this comment.
Posting review findings from the local review.
| .type(ElementMatchers.named("java.net.HttpURLConnection")) | ||
| .transform((b, typeDescription, classLoader, module, protectionDomain) -> | ||
| b.visit(Advice.to(GetResponseCodeAdvice.class) | ||
| .on(ElementMatchers.named("getResponseCode"))) |
There was a problem hiding this comment.
[P2] Capture HttpURLConnection requests that skip getResponseCode
For HttpURLConnection callers that trigger the request with getInputStream() or getErrorStream() and never call getResponseCode(), this is the only advised method, so a 4xx/5xx response (or the IOException thrown by getInputStream() on 4xx) is never recorded. This leaves a common HttpURLConnection usage path outside the promised automatic network-error capture.
brianr
left a comment
brianr
left a comment
There was a problem hiding this comment.
Requesting changes on the current head. The inline comments cover four release blockers. Two existing review threads also still apply, so I have not duplicated them: the agent still records Rollbar's own SyncSender failures, and NetworkEventBridge.composeUrl() still treats :// inside a relative URI's query/path as an absolute URI. The former thread is marked resolved even though no suppression guard is present at this head.
There was a problem hiding this comment.
Beyond the two nit-level findings from this run's inline comments, I also checked a few other candidate issues and ruled them out: instrumenting the Rollbar SDK's own SyncSender does not create a feedback loop, the repeated classloader/bridge-lookup boilerplate across the four HttpUrlConnectionInstrumentation advice classes is a style-only duplication (not a bug), the README's "zero-code-change" wording is accurate for the HTTP call-site instrumentation itself, and the HC4/HC5 hierarchy-walk name filter excluding only JDK prefixes is a performance micro-optimization rather than a correctness gap.
Extended reasoning...
This run's findings are both nits (a redirect-misattribution edge case in the JDK HttpClient advice, and an unnecessary-reflection quality nit in HttpUrlConnectionInstrumentation), and the PR has already absorbed a long sequence of substantive P1/P2 fixes from prior review rounds (recursive getResponseCode() guard, doExecute()-based Apache HC4/HC5 unification covering the HttpHost overloads, UrlSanitizer host-corruption and encoded-credential fixes, composeUrl scheme-position bounding, shading only Byte Buddy instead of the Rollbar SDK, Byte Buddy version bump for current JDKs, and preventing the thin jar from clobbering the shaded one). Given this module's complexity (bytecode instrumentation across four HTTP clients) and its security-sensitive URL-sanitization logic, I'm not approving outright, but wanted to record the additional items examined and ruled out this run so they aren't re-explored from scratch.
| if (response != null) { | ||
| int statusCode = (Integer) response.getClass().getMethod("statusCode").invoke(response); | ||
| if (statusCode >= 400) { | ||
| Object uri = request.getClass().getMethod("uri").invoke(request); | ||
| String method = (String) request.getClass().getMethod("method").invoke(request); | ||
| // response object is the dedup key — unique per send() call, shared between | ||
| // HttpClientFacade and HttpClientImpl so only one event is recorded | ||
| bridge.getMethod("recordNetworkEvent", | ||
| Object.class, String.class, String.class, String.class) | ||
| .invoke(null, response, method, uri.toString(), String.valueOf(statusCode)); |
There was a problem hiding this comment.
🟡 The JDK HttpClient advice (SendAdvice.onExit sync path and NetworkEventBridge.createAsyncCallback async path) reads the URL/method off the original pre-redirect request argument instead of the response, so when the client is configured with a redirect policy other than the default NEVER (e.g. Redirect.NORMAL), a final-hop 4xx/5xx is recorded with the original request's host/method rather than the actual failing one — unlike the other three instrumented clients, whose instrumentation point already sees the final target. Fix by using response.uri() and response.request().method() instead of the request argument in both SendAdvice.onExit (JavaHttpClientInstrumentation.java:132-138) and the HttpResponse branch of createAsyncCallback (NetworkEventBridge.java).
Extended reasoning...
What's wrong: SendAdvice.onExit (JavaHttpClientInstrumentation.java:129-138) records telemetry using request.getClass().getMethod("uri").invoke(request) and request.getClass().getMethod("method").invoke(request), where request is @Advice.Argument(0) — the original HttpRequest object passed into HttpClient.send(...), not anything derived from the returned response. The async path (SendAsyncAdvice → NetworkEventBridge.createAsyncCallback) has the identical pattern: the callback closes over the original request object captured at sendAsync() time and reads uri()/method() off of it in the response.statusCode() >= 400 branch.
Why this is wrong: java.net.http.HttpClient, when configured with a redirect policy other than the default Redirect.NEVER (e.g. HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NORMAL)), follows the entire redirect chain internally inside one send()/sendAsync() call. The javadoc for HttpResponse.uri() says the returned URI "may be different from the request URI if redirection occurred," and HttpResponse.request() returns the actual final HttpRequest (whose method can also change — a 303 converts POST to GET). So when the chain ends in a 4xx/5xx, the response correctly reflects the final hop, but the code reads the pre-redirect request's URI/method instead.
Why nothing else in the code catches this: there is no logic anywhere in SendAdvice/createAsyncCallback that inspects redirect history or consults response.request() — the response is used only for statusCode() and as the WeakHashMap dedup key, never for its own uri()/request().
Step-by-step proof:
- App code:
HttpClient client = HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NORMAL).build(); client.send(HttpRequest.newBuilder(URI.create("https://api.example.com/widgets")).build(), ...).api.example.comresponds 301 →Location: https://cdn.example.com/widgets.- The JDK client follows the redirect internally (still inside the single
send()call) and issues the request tocdn.example.com. cdn.example.comresponds 500.send()returns the finalHttpResponse, whoseuri()ishttps://cdn.example.com/widgetsand whoserequest()is the request that was actually sent tocdn.example.com.SendAdvice.onExitfires with@Advice.Argument(0) requeststill bound to the original request object built in step 2 (URIhttps://api.example.com/widgets), and readsrequest.uri()/request.method()from it.- Recorded telemetry:
{method: <original>, url: "https://api.example.com/widgets", status_code: "500"}— attributing the 500 toapi.example.com, when the failing dependency was actuallycdn.example.com.
Impact: this defeats the purpose of the URL field, which exists specifically to let a developer identify which downstream host actually failed. A developer investigating the error would look at (and possibly page/alert on) the wrong service.
Fix: in SendAdvice.onExit, replace request.uri()/request.method() with response.uri() and response.request().method() (both available via reflection on the HttpResponse/HttpRequest interfaces, consistent with how the rest of the advice already does reflective lookups). The same substitution applies to the HttpResponse branch inside NetworkEventBridge.createAsyncCallback.
Severity: nit, not normal — it requires a non-default, opt-in redirect policy (the JDK's own default is Redirect.NEVER, under which a 3xx is simply not recorded at all) combined with a redirect chain that terminates in an error. The failure mode is misattributed/degraded telemetry, not a crash, exception, or data loss, and the misattributed URL is still a URL the app genuinely requested (just the wrong hop). This is consistent with how the similarly-scoped composeUrl nested-URL host-misattribution finding elsewhere in this PR was rated.
| if (statusCode >= 400) { | ||
| Object url = connection.getClass().getMethod("getURL").invoke(connection); | ||
| String urlStr = url != null ? url.toString() : ""; | ||
| String method = (String) connection.getClass() | ||
| .getMethod("getRequestMethod").invoke(connection); | ||
| bridge.getMethod("recordNetworkEvent", | ||
| Object.class, String.class, String.class, String.class) | ||
| .invoke(null, connection, method, urlStr, String.valueOf(statusCode)); |
There was a problem hiding this comment.
🟡 GetResponseCodeAdvice.onExit() (and the equivalent code in GetInputStreamAdvice/GetErrorStreamAdvice) reflects into getURL()/getRequestMethod()/getResponseCode() via connection.getClass().getMethod(...).invoke(...), but these are public methods declared directly on the bootstrap-loaded java.net.HttpURLConnection/URLConnection, visible from every classloader — unlike the genuine NetworkEventBridge lookup in the same methods, which does need reflection to cross into the app classloader. Typing @Advice.This as HttpURLConnection and calling the methods directly removes six reflective lookups plus their checked-exception handling from a per-error-response path with identical behavior, since ByteBuddy inlines the advice regardless of the declared parameter type.
Extended reasoning...
What the finding is
GetResponseCodeAdvice.onExit (lines 171-178) reflectively invokes getURL() and getRequestMethod() on the HttpURLConnection instance via connection.getClass().getMethod(name).invoke(connection). The same pattern recurs for getResponseCode() in GetInputStreamAdvice.onExit (line 94) and GetErrorStreamAdvice.onExit (line 125).
All three methods — getResponseCode() and getRequestMethod() on java.net.HttpURLConnection, getURL() on its superclass java.net.URLConnection — are public and declared on bootstrap-loaded java.base classes. There is no classloader gap to bridge: every type this advice is inlined into (HttpURLConnection itself, or a concrete subclass like sun.net.www.protocol.http.HttpURLConnection) is a subtype of the bootstrap class HttpURLConnection, so a direct invokevirtual reference resolves from any classloader. This is fundamentally different from the NetworkEventBridge lookup a few lines below in the same methods, which genuinely must go through Thread.currentThread().getContextClassLoader().loadClass(...) because NetworkEventBridge lives in the application classloader and is invisible from a bootstrap-inlined advice body.
Why the change is safe
Typing @Advice.This as HttpURLConnection instead of Object is valid for every instrumented site: GetResponseCodeAdvice is inlined directly into java.net.HttpURLConnection.getResponseCode() (exact match), and GetInputStreamAdvice/GetErrorStreamAdvice target concrete subtypes, for which HttpURLConnection is always an assignable supertype. ByteBuddy inlines advice bytecode into the target method regardless of the advice parameter's declared type, so this is a purely mechanical substitution — connection.getResponseCode()/getURL()/getRequestMethod() called directly instead of via Method.invoke. getURL() and getRequestMethod() declare no checked exceptions, and getResponseCode()'s IOException is already caught by the surrounding catch (Throwable ignored), so no new exception handling is needed at the call site.
Step-by-step proof (GetResponseCodeAdvice)
- A 404 response triggers
getResponseCode()to return,statusCode >= 400. - Current code:
connection.getClass().getMethod("getURL").invoke(connection)— a reflective lookup + invoke against a bootstrap class the calling code could reference directly. - Replacement:
connection.getURL()— same bytecode-inlined call site, same return value, no reflection, noNoSuchMethodException/IllegalAccessException/InvocationTargetExceptionhandling needed. - Identical for
getRequestMethod(), and forgetResponseCode()in the two other advice classes.
Impact
Six reflective getMethod()+invoke() calls are removed from the response/error-handling path (fired on every 4xx/5xx HttpURLConnection response), improving clarity and per-response cost with no behavior change. It is also strictly safer under JPMS than reflecting on an internal JDK implementation class. This is a pure code-quality/efficiency cleanup — nothing about program correctness changes — so it does not block merge.
| private static void installInstrumentation(Instrumentation inst) { | ||
| // Override ByteBuddy's default which ignores all java.* and javax.* classes, | ||
| // so we can instrument JDK HTTP clients (HttpURLConnection, HttpClient). | ||
| // We still ignore ByteBuddy's own classes to avoid instrumentation loops. | ||
| AgentBuilder builder = new AgentBuilder.Default() | ||
| .ignore(ElementMatchers.nameStartsWith("net.bytebuddy.") | ||
| .or(ElementMatchers.nameStartsWith("com.rollbar.agent.shaded."))) | ||
| .with(new ErrorReportingListener()) | ||
| .with(AgentBuilder.InitializationStrategy.NoOp.INSTANCE) | ||
| .with(AgentBuilder.TypeStrategy.Default.REDEFINE); |
There was a problem hiding this comment.
🟡 installInstrumentation() (RollbarAgent.java:40-49) never sets .with(AgentBuilder.RedefinitionStrategy.RETRANSFORMATION), so ByteBuddy stays at the default RedefinitionStrategy.DISABLED and only transforms classes loaded after installOn(inst) runs — it never retransforms classes already loaded. This silently breaks the dynamic-attach path exposed via agentmain (and the manifest's Can-Redefine-Classes/Can-Retransform-Classes: true), since HTTP client classes are almost always already loaded in a running JVM by the time attach happens, so none of the four instrumentations install and zero telemetry is recorded with no error raised.
Extended reasoning...
What the bug is: RollbarAgent.installInstrumentation() builds its AgentBuilder (lines 40-49) with an ignore filter, the ErrorReportingListener, InitializationStrategy.NoOp, and TypeStrategy.Default.REDEFINE — but never calls .with(AgentBuilder.RedefinitionStrategy...). ByteBuddy's AgentBuilder.Default defaults RedefinitionStrategy to DISABLED. Under DISABLED, installOn(inst) registers the ClassFileTransformer with inst.addTransformer(transformer, /* canRetransform */ false) — the transformer only sees types as they are freshly loaded from that point forward. It never iterates over and retransforms classes the JVM already has loaded at the moment installOn runs. Note TypeStrategy.Default.REDEFINE (which is set) is a different, independent setting — it controls how a matched type's bytecode is rewritten, not whether already-loaded types get revisited — so it does not compensate for the missing RedefinitionStrategy.\n\nThe code path that triggers it: RollbarAgent exposes a public agentmain(String, Instrumentation) (lines 36-38) specifically for dynamic attach to an already-running JVM via VirtualMachine.loadAgent(...), and build.gradle.kts's manifest sets both Agent-Class and Can-Redefine-Classes/Can-Retransform-Classes: true — JVM-level permissions that exist for exactly this scenario. But in a real running application at the moment of attach, java.net.HttpURLConnection, java.net.http.HttpClient, and any already-used Apache HttpClient classes are, in virtually every case, already loaded. With RedefinitionStrategy left at DISABLED, none of the four installIfAvailable/install calls in installInstrumentation can retransform those already-loaded classes, so no advice is ever woven into them.\n\nWhy nothing else catches it: ErrorReportingListener.onError is the only failure-surfacing mechanism in this code, but it only fires when a transform attempt is made and fails to apply — here, no transform is even attempted on the already-loaded classes, so onError never fires. The agent's agentmain returns normally, giving every outward signal of successful attachment while silently receives zero events for the rest of the process's life.\n\nStep-by-step proof:\n1. A long-running application is already executing, having triggered classloading of sun.net.www.protocol.http.HttpURLConnection (or 's impl classes) well before any Rollbar tooling attaches.\n2. An operator (or tooling) dynamically attaches this agent via VirtualMachine.attach(pid).loadAgent(jarPath), invoking RollbarAgent.agentmain(args, inst).\n3. installInstrumentation(inst) builds the AgentBuilder and calls HttpUrlConnectionInstrumentation.install(builder, inst) (and the other three installIfAvailable calls), each ending in .installOn(inst).\n4. Because RedefinitionStrategy was never set, installOn calls inst.addTransformer(transformer, false) — false meaning "do not retransform currently loaded classes."\n5. The already-loaded HttpURLConnection/HttpClient/Apache HC classes are never revisited by the JVM; no transform is attempted, so ErrorReportingListener.onError never fires.\n6. The application continues making HTTP calls through those already-loaded, un-instrumented classes. Every 4xx/5xx response goes completely unrecorded — AgentTelemetryStore stays empty for the process's entire remaining lifetime, with no log line or exception anywhere indicating the failure.\n\nHow to fix: add .with(AgentBuilder.RedefinitionStrategy.RETRANSFORMATION) to the builder in installInstrumentation(). The manifest already grants the required Can-Retransform-Classes: true JVM permission, so this is a pure one-line fix with no other changes needed; advice inlining here only rewrites method bodies, which retransformation supports without any schema/field changes.\n\nSeverity: the documented and tested path is -javaagent:/premain, where HTTP classes are loaded lazily after premain installs the transformer — this is proven by the passing WireMock integration test suite. Dynamic attach via agentmain is not documented in the README, even though the code (agentmain) and manifest (Agent-Class, Can-Redefine-Classes/Can-Retransform-Classes) both advertise it as a supported entry point. Since merging without this fix does not break the primary, documented feature, this should not block the PR — but it is a genuine, reproducible defect on an entry point the module explicitly exposes and grants JVM permissions for, and is worth a one-line fix (or removing the agentmain/Agent-Class surface if dynamic attach isn't actually meant to be supported yet).
| } | ||
|
|
||
| if (response != null && request != null) { | ||
| int statusCode = response.getStatusLine().getStatusCode(); | ||
| if (statusCode >= 400) { | ||
| // The host-based overloads carry the target separately from a request whose URI may be | ||
| // just a path, so rejoin the two rather than reading the request URI alone. | ||
| String base = target != null ? target.toURI() : null; | ||
| String requestUri = request.getRequestLine() != null | ||
| ? request.getRequestLine().getUri() : null; | ||
| NetworkEventBridge.recordNetworkEvent( | ||
| response, | ||
| request.getRequestLine().getMethod(), | ||
| NetworkEventBridge.composeUrl(base, requestUri), | ||
| String.valueOf(statusCode) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 The doExecute(HttpHost, request, context) advice reads its recorded host/URI off the original @Advice.Argument(0)/(1) parameters, but Apache HttpClient (both HC4 and HC5) follows redirects internally via RedirectExec, which reassigns only its own local currentRequest/currentRoute and never mutates the objects doExecute is holding. So when a request to host A 3xx-redirects to host B and host B returns a 4xx/5xx, the recorded telemetry pairs the final status code with host A instead of the failing host B. This is a pre-existing design limitation, unrelated to any change introduced by this PR — it stems from choosing doExecute() as the single instrumentation point, which is otherwise the right choice for overload coverage.
Extended reasoning...
What the bug is. DoExecuteAdvice.onExit in ApacheHttpClient4Instrumentation.java (lines 100-117) binds @Advice.Argument(0) target and @Advice.Argument(1) request — the exact parameters doExecute(HttpHost, HttpRequest, HttpContext) was originally invoked with — and uses them, together with @Advice.Return response, to build the recorded URL via NetworkEventBridge.composeUrl(target.toURI(), request.getRequestLine().getUri()). ApacheHttpClient5Instrumentation.java's DoExecuteAdvice has the identical shape, reading request.getUri()/request.getMethod() off its own doExecute argument.
Why this is wrong. InternalHttpClient.doExecute() (verified via javap on httpclient-4.5.14) wraps the incoming request into a fresh HttpRequestWrapper and hands it to the exec chain (execChain.execute(route, wrapper, context)); it never reassigns its own target/request locals afterward. RedirectExec, the outermost element of that chain, is what actually follows a redirect: on each hop it reassigns only its own local currentRequest/currentRoute (via HttpRequestWrapper.wrap(redirect)), then returns the final response from its loop. It never touches the target/request objects that doExecute itself is holding. So for a redirect chain from host A to host B ending in a 4xx/5xx, doExecute() returns the true final (host B) response, but its own target/request parameters — the ones the advice reads — still describe host A. HC5's InternalHttpClient/RedirectExec follow the same wrap-and-reassign-local pattern, so ApacheHttpClient5Instrumentation has the same issue.
Reachability. This is on the default path: HttpClients.createDefault() enables DefaultRedirectStrategy, so HC4/HC5 follow redirects out of the box for GET/HEAD (and 307/308 for any method). Any followed redirect that ends in an error — cross-host, http→https, apex→www, a CDN hop — records the pre-redirect host/URL paired with the final status code.
Why nothing else in the code catches it. The advice never consults HttpClientContext (available as @Advice.Argument(2), and updated by RedirectExec to the final HTTP_TARGET_HOST/HTTP_REQUEST attributes), and ClassicHttpResponse/HttpResponse carry no back-reference to the request actually sent — so there is no way to recover the final target from the response alone, unlike java.net.http.HttpResponse, which does expose response.request().
Step-by-step proof.
client.execute(new HttpGet("http://a.example.com/widgets"))wherea.example.com301-redirects tohttps://b.example.com/widgets.doExecute(target=a.example.com, request=GET /widgets, context)is invoked;target/requestare bound as this method's own arguments.RedirectExec.execute()follows the 301 by reassigning its own localcurrentRequest/currentRoutetob.example.com, issues the redirected request, and gets back a 500.doExecute()returns that 500HttpResponse— but itstarget/requestparameters are unchanged, stilla.example.com/GET /widgets.DoExecuteAdvice.onExitfires withtarget=a.example.com,request=GET /widgets,response=500, records{method: GET, url: "http://a.example.com/widgets", status_code: "500"}.- The telemetry attributes the failure to
a.example.com, when the request that actually failed went tob.example.com.
Impact and fix. This degrades (misattributes) the recorded URL/host for a redirected error — the status code is still correct, and no crash/data loss occurs, so it is a nit rather than a blocking issue. A fix would read the final target/request from HttpClientContext's HTTP_TARGET_HOST/HTTP_REQUEST attributes (updated by the exec chain on each redirect hop) instead of from doExecute's own arguments. This is distinct from the already-reported JavaHttpClientInstrumentation redirect finding (comment 2026-08-10T21:29:36Z): that finding's aside that HC4/HC5 "already see the final target" is incorrect, and its suggested fix (response.request()) does not apply here since Apache's ClassicHttpResponse has no such back-reference.
| if (thrown != null) { | ||
| Boolean recorded = (Boolean) bridge | ||
| .getMethod("markAsRecorded", Object.class).invoke(null, thrown); | ||
| if (recorded) { | ||
| String msg = thrown.getMessage() != null | ||
| ? thrown.getMessage() : thrown.getClass().getName(); | ||
| bridge.getMethod("recordError", String.class).invoke(null, msg); | ||
| } | ||
| return; |
There was a problem hiding this comment.
🟡 Nit: the reflective markAsRecorded/recordError block is copy-pasted verbatim in three advice sites — HttpUrlConnectionInstrumentation.GetResponseCodeAdvice.onExit (lines 160-168), JavaHttpClientInstrumentation.SendAdvice.onExit, and SendAsyncAdvice.onExit — each doing two separate reflective Method lookups plus the same null-message fallback. A single NetworkEventBridge.recordThrowable(Throwable) helper would collapse each site to one reflective invoke, with no behavior change.
Extended reasoning...
What's duplicated: the same 5-line pattern — reflectively call markAsRecorded(thrown), and if it returns true, build a null-safe message (thrown.getMessage() != null ? thrown.getMessage() : thrown.getClass().getName()) and reflectively call recordError(message) — is copy-pasted verbatim across three advice classes: HttpUrlConnectionInstrumentation.GetResponseCodeAdvice.onExit (lines 160-168), JavaHttpClientInstrumentation.SendAdvice.onExit (~118-126), and JavaHttpClientInstrumentation.SendAsyncAdvice.onExit (~65-73). Each site performs two independent reflective getMethod()+invoke() calls (one for markAsRecorded, one for recordError) plus the identical message-fallback branch.\n\nThe same shape also shows up, non-reflectively, in the two Apache HC4/HC5 DoExecuteAdvice classes and in NetworkEventBridge.createAsyncCallback's thrown-exception branch — but the three reflective copies alone are enough to justify a shared helper.\n\nWhy this happened: each advice class is inlined into a different bootstrap/JDK class (HttpURLConnection, HttpClient's send/sendAsync) and has to cross the classloader gap to reach NetworkEventBridge, which lives in the application classloader. Because the crossing itself requires reflection, it was natural to write the whole mark+record sequence reflectively at each call site rather than factoring it into the bridge — but the branch and string logic don't need to be reflective at all; only the single entry-point call into does.\n\nThe fix: add one method to NetworkEventBridge — e.g. public static void recordThrowable(Throwable thrown) — that does the markAsRecorded check, builds the null-safe message, and calls recordError internally (all in the app classloader, no reflection needed for that part). Each of the three advice sites then reduces from five duplicated lines plus two reflective Method objects down to a single reflective invoke:\n\njava\nbridge.getMethod("recordThrowable", Throwable.class).invoke(null, thrown);\n\n\nStep-by-step proof this is safe and behavior-preserving:\n1. Today, GetResponseCodeAdvice.onExit does: look up markAsRecorded, invoke it with thrown → if true, look up recordError, build the message, invoke it.\n2. With the helper, NetworkEventBridge.recordThrowable(thrown) runs the identical two-step check but as plain Java inside the bridge class (which already has direct access to markAsRecorded/recordError since they're static methods on the same class).\n3. The advice site now looks up and invokes only recordThrowable, passing thrown through unchanged.\n4. Since recordThrowable's internal logic is byte-for-byte the same branch/message-building code that used to live at the call site, the recorded telemetry (message content, dedup behavior via markAsRecorded) is identical — only the amount of reflection and duplicated code changes.\n\nImpact: this is a pure code-quality/reuse cleanup — three (arguably five, counting the HC4/HC5 direct-call sites) copies of the same logic collapse to one, each reflective call site shrinks from ~8 lines and two Method lookups to one, and there's no behavior change. It doesn't block merging.
| } | ||
|
|
||
| /** | ||
| * Joins a base URI with a request URI, for clients that dispatch a target host separately from a | ||
| * request whose URI may be relative. | ||
| * | ||
| * <p>Apache HC's {@code doExecute(HttpHost, request, context)} receives the target host as its | ||
| * own argument, so a request issued through the host-based {@code execute(HttpHost, request)} | ||
| * overloads carries only a path (e.g. {@code /charge}). Rejoining the two is what keeps the host | ||
| * in the recorded URL. A request URI that is already absolute is returned untouched, and a null | ||
| * base (HC leaves the target null for a relative URI it could not resolve) degrades to the path | ||
| * alone. | ||
| * | ||
| * @param baseUri the target host as a URI (e.g. {@code https://api.example.com}), or null | ||
| * @param requestUri the request URI, absolute or relative, or null | ||
| * @return the joined URL — never null, so the caller always has something to sanitize | ||
| */ | ||
| public static String composeUrl(String baseUri, String requestUri) { |
There was a problem hiding this comment.
🟡 NetworkEventBridge.createAsyncCallback resolves Class.forName("java.net.http.HttpResponse") and Class.forName("java.net.http.HttpRequest") fresh inside the BiConsumer body, which fires on every completed async HTTP response (not only error ones, since resolving HttpResponse is needed just to read statusCode()).
Extended reasoning...
NetworkEventBridge.createAsyncCallback (lines 124-141) returns a BiConsumer that SendAsyncAdvice.onExit chains onto the future via whenComplete(...). That callback body runs once for every completed sendAsync() call. Inside it, Class.forName("java.net.http.HttpResponse") and Class.forName("java.net.http.HttpRequest") are re-resolved on every invocation, and — because statusCode() itself has to be invoked through httpResponseIface before the >= 400 gate is even checked — both lookups execute for every async response, including ordinary 2xx ones, not just failures.
Both types are fixed java.base interfaces that are always present and resolvable at agent-load time (the module's own build targets Java 11+, and java.net.http is an exported package per the comment already in this file). There is no case where the class identity of HttpResponse/HttpRequest could legitimately change between calls, so redoing the Class.forName lookup (a caller-sensitive stack walk plus a classloader loadClass call, even once the class is already loaded and initialized) on every response is unnecessary repeated work in what is effectively a hot path for any application making frequent async HTTP calls.
The fix is to resolve both Class<?> objects once and reuse them across invocations — either as fields resolved once per createAsyncCallback call (safest, since this method is only reached from the sendAsync advice path where java.net.http is guaranteed present) or as static final fields if NetworkEventBridge is confirmed to only ever be reached via that path. This is a pure efficiency cleanup: it does not change what gets recorded, when, or how — the resolved Class objects are identical either way — so it carries no behavioral risk.
Step-by-step proof:
- App code calls
httpClient.sendAsync(request, bodyHandler)twice in a row for two different requests that both return200 OK. SendAsyncAdvice.onExitchains a callback (fromcreateAsyncCallback) onto each returned future viawhenComplete.- Both callbacks fire, once per completed future. Each independently executes
Class.forName("java.net.http.HttpResponse")andClass.forName("java.net.http.HttpRequest")— two full lookups per response, four total for these two unrelated 2xx calls that never even reach thestatusCode >= 400branch. - Hoisting the resolution out of the lambda body means the two
Class.forNamecalls happen once (atcreateAsyncCallbackinvocation time, or once for the JVM lifetime if made static), and the returned callback just references the already-resolvedClassobjects — identical behavior, less repeated work per response.
Severity is nit: this is code-quality/efficiency cleanup only, with no correctness impact, and doesn't block merging.
| private static Object invokeVia(String apiTypeName, Object receiver, String methodName) | ||
| throws ReflectiveOperationException { | ||
| ClassLoader classLoader = receiver.getClass().getClassLoader(); | ||
| Class<?> apiType = Class.forName(apiTypeName, false, | ||
| classLoader != null ? classLoader : ClassLoader.getSystemClassLoader()); | ||
| return apiType.getMethod(methodName).invoke(receiver); |
There was a problem hiding this comment.
🟡 ApacheHttpClient4Instrumentation.invokeVia (lines 113-118) and ApacheHttpClient5Instrumentation.invokeVia (lines 119-124) are byte-for-byte identical private reflection helpers, existing for the same reason in both files. Consider hoisting the shared logic into a common location (e.g. NetworkEventBridge) so a future fix doesn't need to be applied twice; this is a nit, not a blocker.
Extended reasoning...
ApacheHttpClient4Instrumentation.invokeVia (lines 113-118) and ApacheHttpClient5Instrumentation.invokeVia (lines 119-124) are byte-for-byte identical private static helpers. Both have the exact same signature — private static Object invokeVia(String apiTypeName, Object receiver, String methodName) throws ReflectiveOperationException — and the exact same three-statement body: resolve receiver.getClass().getClassLoader() (falling back to the system classloader if null), Class.forName(apiTypeName, false, classLoader), then apiType.getMethod(methodName).invoke(receiver).
Both copies exist for the identical structural reason: recordResponse in each instrumentation class needs to call methods on Apache HTTP objects (HttpResponse, StatusLine, HttpRequest/RequestLine in HC4; the hc5 equivalents in HC5) without the advice/support code naming those types directly at compile time in a way that could break classloading in embedding scenarios described elsewhere in this PR's review — so both classes fall back to reflecting through the receiver's own classloader.
Because the helper is fully generic — its own signature only mentions String/Object, never an org.apache.http/org.apache.hc type — nothing forces it to live inside the per-HTTP-client-version class. Both ApacheHttpClient4Instrumentation.recordResponse and ApacheHttpClient5Instrumentation.recordResponse already call NetworkEventBridge static methods directly (recordNetworkEvent, composeUrl), so NetworkEventBridge is the natural shared home; a public static method there (package-private wouldn't be reachable across the instrumentation package boundary) would let both instrumentation classes delegate instead of maintaining separate copies.
The risk of leaving this as-is is that a future change to the lookup strategy — for example, caching the Method object across calls instead of re-resolving it via getMethod() on every 4xx/5xx response, which another reviewer flagged as a possible efficiency improvement — would have to be remembered and applied in two files. Missing one during that kind of edit is an easy, quiet way to introduce version-specific behavior drift between the HC4 and HC5 instrumentations for no reason tied to any real difference between the two libraries.
Step-by-step proof of the duplication:
- Open
ApacheHttpClient4Instrumentation.java:113-118— seeinvokeVia(String, Object, String)resolving classloader, callingClass.forName, thengetMethod().invoke(). - Open
ApacheHttpClient5Instrumentation.java:119-124— the method body is character-for-character the same aside from surrounding javadoc referencing "HC 5.x" vs "HC 4.x". - Grep both files for
org.apacheinsideinvokeVia— neither method references a library type; the type name is always passed in as a caller-suppliedString. - Confirm both call sites (
recordResponsein each file) already depend onNetworkEventBridge, showing that's the natural shared location without requiring a new file.
This is a pure reuse/DRY cleanup with no behavior change — extracting the method does not alter what gets recorded or how classloading is resolved, so it doesn't block merging. It's flagged as a nit for a maintainer to fold into a follow-up rather than something that needs to hold up this PR.
There was a problem hiding this comment.
No bugs were found in this run. Given the scope and complexity of this change — a new bytecode-instrumentation agent with several JVM classloader edge cases — I'm deferring to a human reviewer rather than approving outright.
What was reviewed: the four HTTP client instrumentations (HttpURLConnection, java.net.http.HttpClient, Apache HC4/HC5) and their advice-to-bridge reflection paths, URL sanitization/credential-stripping, event dedup logic, and the Gradle shading/JDK-version setup. Two candidate issues were examined and ruled out: composeUrl's handling of protocol-relative request URIs, and the module-level (not per-Rollbar-instance) scope of the telemetry store.
Extended reasoning...
Overview
This PR adds a new rollbar-java-agent module: a -javaagent that uses ByteBuddy to instrument HttpURLConnection, java.net.http.HttpClient, and Apache HttpClient 4.x/5.x, recording 4xx/5xx responses and connection failures as Rollbar telemetry events. It includes URL sanitization, WeakHashMap-based dedup for re-entrant calls, a shaded build with only ByteBuddy relocated, and WireMock-backed integration tests for each client.
Security risks
The main security-relevant surface is UrlSanitizer, which strips userinfo/credentials, query params, and fragments before URLs are recorded as telemetry. Prior review rounds found and fixed real gaps here (encoded '@' in userinfo, unescaped-space fallback paths, underscore hostnames) and those fixes are present in the current code with dedicated regression tests. No injection, auth, or crypto surface is introduced elsewhere.
Level of scrutiny
This warrants more than a mechanical-change level of review: it's a brand-new module built on bytecode instrumentation and reflection across classloader boundaries, which is inherently subtle (as shown by the volume of substantive findings across this PR's review history — advice type resolution across classloaders, redefinition/ignore-matcher semantics, re-entrant call dedup, JDK/ByteBuddy version compatibility). It doesn't touch any existing production code path in rollbar-java itself, which lowers blast radius, but the agent module's own correctness is nontrivial.
Other factors
Comparing against the PR's history, every previously reported P1/blocking issue from both human review (brianr) and prior automated runs (JVM version support, shading the Rollbar SDK unrelocated, the zero-code-change claim, encoded-credential stripping, Apache advice NoClassDefFoundError risk, ignore-matcher dropping isSynthetic, connection-failure dedup) has a corresponding follow-up commit in the current code. The remaining open items visible in the timeline are nit-severity code-duplication suggestions, not correctness bugs. Test coverage is substantial (WireMock integration tests per client, dedicated URL sanitizer and composeUrl unit tests). Given the module is new and the domain is unusually easy to get subtly wrong, a human sign-off is still the safer default.
AI Agent Review (openai, openai-astra)Reviewed the whole new Overall this is careful work, and the things that usually go wrong in a ByteBuddy agent are handled. The rationale comments mostly check out against the code I read:
The largest finding is that the agent instruments the SDK's own uploader. The rest are small. Other notes I did not raise as findings:
I did not run the build or the tests, so nothing here asserts that they pass. |
|
@buongarzoni thoughts on this? Claude was able to reproduce this issue, and claims the majority of the sample apps would be affected by it. The JVM refuses to start unless the Rollbar SDK is on the system classpath. This is only when the new agent is configured. |
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether running multiple Rollbar instances in one JVM would cause the agent's shared static telemetry buffer (AgentTelemetryStore) to duplicate agent-captured network events across each instance's reports. It would — getAll() returns a non-destructive snapshot, so every reader sees the same buffered events — but only as duplicated (not lost or corrupted) telemetry in an uncommon multi-Rollbar-instance setup, so I'm not raising it as a separate blocker here.
Extended reasoning...
This run's inline findings (UrlSanitizer.java fallback userinfo stripping using the wrong '@' and AgentTelemetryEventTracker permanently giving up on the agent lookup after one failure) are new/updated results, not restated here. I additionally verified from the code that AgentTelemetryStore.getAll() (rollbar-java-agent/src/main/java/com/rollbar/agent/AgentTelemetryStore.java:112) is non-destructive and static, so concurrent Rollbar instances in one JVM would each read and report the same agent events — a real but low-severity, edge-case behavior (duplication, not data loss), which is why it was ruled out rather than flagged.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| private Method resolve() { | ||
| Method resolved = getAll; | ||
| if (resolved != null || lookupFailed) { | ||
| return resolved; | ||
| } | ||
| // Benign race: two threads may both resolve, and land on the same Method. | ||
| try { | ||
| Class<?> store = ClassLoader.getSystemClassLoader().loadClass(AGENT_STORE_CLASS); | ||
| resolved = store.getMethod(AGENT_STORE_METHOD); | ||
| getAll = resolved; | ||
| return resolved; | ||
| } catch (ClassNotFoundException e) { | ||
| LOGGER.info("The Rollbar Java agent is not attached to this JVM; only telemetry events " | ||
| + "recorded by the application will be reported. Add -javaagent:<rollbar-java-agent " | ||
| + "jar> to capture HTTP errors automatically."); | ||
| } catch (Exception e) { | ||
| LOGGER.warn("The Rollbar Java agent is attached but its telemetry store could not be " | ||
| + "read; check that the agent and rollbar-java versions match", e); | ||
| } | ||
| lookupFailed = true; |
There was a problem hiding this comment.
🟡 (optional) If the Rollbar agent is attached dynamically (via agentmain, RollbarAgent.java:40) after the tracker's first lookup, agent telemetry is lost for the rest of the process's life. SystemClassLoaderAgentEventSource.resolve() sets lookupFailed = true permanently on the first ClassNotFoundException and never retries, so a later successful attach is never noticed. Fix: retry the classloader lookup on subsequent getAll() calls instead of caching failure forever (e.g. re-check periodically or drop the permanent latch), while still avoiding a lookup on every call in the common no-agent case.
Why this was flagged
AgentTelemetryEventTracker.getAll() is called before the agent is dynamically attached (e.g. app reports an error early, or the agent is attached later via VirtualMachine.attach + agentmain — a path RollbarAgent.java:40 explicitly supports). resolve() throws ClassNotFoundException, logs once, and sets lookupFailed=true (line 267). Once the agent later attaches and starts recording into AgentTelemetryStore, resolve() still short-circuits at line 250 (if (resolved != null || lookupFailed) return resolved;) and returns null forever, so getAll() never reads agent events again — unlike the base tracker, which has no such permanent latch on a transient failure.
Verification: nit. The mechanism is real and reachable. In SystemClassLoaderAgentEventSource.resolve() (AgentTelemetryEventTracker.java), line 250 short-circuits if (resolved != null || lookupFailed) { return resolved; } and line 267 unconditionally sets lookupFailed = true after a ClassNotFoundException; lookupFailed (line 231) is a volatile boolean that is never reset. So the very first getAll() that…
There was a problem hiding this comment.
Beyond the confirmed inline finding, this run also checked two adjacent risks in the new AgentTelemetryEventTracker: a malformed/version-skewed agent event doesn't break report submission (toTelemetryEvent drops bad events individually rather than throwing), and the shared app+agent telemetry cap in getAll() is a documented, intentional tradeoff rather than an unbounded-eviction bug. Neither is a concern beyond what's already flagged inline.
Extended reasoning...
Reviewed the newly introduced rollbar-java-agent module (ByteBuddy-based HTTP instrumentation, in-memory telemetry buffer, cross-classloader reflection bridge) and the consuming AgentTelemetryEventTracker in rollbar-java; this PR has an extensive prior review history including a CONFIRMED new finding this run about AgentTelemetryStore's JVM-wide static event buffer leaking telemetry across co-deployed apps in shared-JVM deployments (Tomcat/WildFly), plus a further verified-but-dropped finding and several still-open threads from earlier rounds. Given the confirmed architectural finding and the volume of unresolved history on this security/data-isolation-sensitive change, this is not a case for approval.
| public AgentTelemetryEventTracker(Provider<Long> timestampProvider, int maximumTelemetryData) { | ||
| this(new RollbarTelemetryEventTracker(timestampProvider, maximumTelemetryData), | ||
| new SystemClassLoaderAgentEventSource(), maximumTelemetryData); | ||
| } | ||
|
|
||
| AgentTelemetryEventTracker(TelemetryEventTracker delegate, AgentEventSource agentEventSource, | ||
| int maximumTelemetryData) { | ||
| this.delegate = delegate; | ||
| this.agentEventSource = agentEventSource; | ||
| this.maximumTelemetryData = maximumTelemetryData; | ||
| } | ||
|
|
||
| /** | ||
| * Get the application's and the agent's events, oldest first, capped at the configured maximum. | ||
| */ | ||
| @Override | ||
| public List<TelemetryEvent> getAll() { | ||
| List<TelemetryEvent> agentEvents = readAgentEvents(); | ||
| List<TelemetryEvent> events = delegate.getAll(); | ||
| if (agentEvents.isEmpty()) { | ||
| return events; | ||
| } | ||
|
|
||
| List<TimestampedEvent> merged = new ArrayList<>(events.size() + agentEvents.size()); | ||
| // The two buffers are each in order, but interleave in time, so the merged timeline has to be | ||
| // sorted. The sort is stable, which keeps same-millisecond events in their recorded order. | ||
| for (TelemetryEvent event : events) { | ||
| merged.add(new TimestampedEvent(event)); | ||
| } | ||
| for (TelemetryEvent event : agentEvents) { | ||
| merged.add(new TimestampedEvent(event)); | ||
| } | ||
| merged.sort(TimestampedEvent.BY_TIMESTAMP); | ||
|
|
||
| int from = Math.max(0, merged.size() - Math.max(maximumTelemetryData, 0)); |
There was a problem hiding this comment.
🟡 (optional) Callers of the public AgentTelemetryEventTracker(Provider, int) constructor with maximumTelemetryData > 100 get more telemetry events per report than the SDK's documented cap allows, unlike the sibling tracker. RollbarTelemetryEventTracker clamps this same parameter to at most MAXIMUM_CAPACITY_FOR_TELEMETRY_EVENTS (100) in its constructor, but AgentTelemetryEventTracker.java:90 stores the raw, unclamped value and getAll() (line 115) uses it directly to size the truncation window, so up to 100 delegate events plus 100 agent events (200 total) can be returned untruncated. Fix: clamp maximumTelemetryData to [0,100] in the constructor the same way RollbarTelemetryEventTracker does, so the per-report event cap holds for both trackers.
Why this was flagged
An integrator calls new AgentTelemetryEventTracker(timestampProvider, 500) (public ctor, AgentTelemetryEventTracker.java:81) instead of the no-arg constructor, e.g. copying a value from ConfigBuilder.maximumTelemetryData. The raw 500 is stored unclamped at line 90 and used at line 115's merged.size() - Math.max(maximumTelemetryData, 0), so from computes to 0 and getAll() returns the full merged list. delegate.getAll() (RollbarTelemetryEventTracker) is separately capped at 100, and AgentTelemetryStore is capped at 100 (AgentTelemetryStore.MAX_EVENTS), so up to 200 events can be returned. On the base branch RollbarTelemetryEventTracker enforces 'closest to [0,100] taken' so payloads never exceed 100 telemetry events; here the safety cap is silently bypassed whenever maximumTelemetryData > 100, only Math.max(...,0) guards the lower bound.
Verification: nit. The mechanism is real and reachable. RollbarTelemetryEventTracker clamps its maximumTelemetryData to Math.min(value, MAXIMUM_CAPACITY_FOR_TELEMETRY_EVENTS)==100 (RollbarTelemetryEventTracker.java:50-54). AgentTelemetryEventTracker's public two-arg ctor (AgentTelemetryEventTracker.java:81-84) hands the raw value to the delegate (clamped there for app events) but ALSO stores the raw,…
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (10):
- 🔴
rollbar-java-agent/src/main/java/com/rollbar/agent/AgentTelemetryStore.java:69—Any operator running several WARs/apps in one JVM (Tomcat, WildFly) gets cross-application telemetry leakage. AgentTele… - Also unresolved: 9 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| String path = uri.getRawPath() != null ? uri.getRawPath() : ""; | ||
| String scheme = uri.getScheme(); | ||
| return (scheme != null ? scheme + "://" : "//") + authority + path; |
There was a problem hiding this comment.
🟡 (optional) Rollbar telemetry can still ship raw credentials when a request URL's PATH embeds another URL with userinfo, e.g. https://gateway.example.com/proxy/https://user:secret@ backend.internal/data. sanitize() (lines 29-43) only strips userinfo from uri.getRawAuthority() and appends uri.getRawPath() verbatim at line 41-43, so a nested scheme://user:pass@ host inside the path is never touched. This hits every well-formed absolute URL captured by the agent's instrumentation (proxy/relay/webhook-forwarding endpoints commonly embed the target URL, including legacy basic-auth creds, in a path segment), unlike the query string, which is dropped entirely. Fix: recursively detect and strip userinfo@ from any nested scheme://...@... …
Why this was flagged
…occurring anywhere in the path, not just the outermost authority, applied identically in this primary path and in fallbackSanitize's authority-bounded stripping.
Trigger: any HTTP call captured by NetworkEventBridge/UrlSanitizer.sanitize() (NetworkEventBridge.java:80) whose URL is well-formed (so it hits the primary branch, not fallbackSanitize) and whose path contains an embedded scheme://user:pass@ host segment, e.g. a reverse-proxy or webhook-relay endpoint. UrlSanitizer.sanitize() strips userinfo only from uri.getRawAuthority() (lines 29-40) and then appends uri.getRawPath() unchanged at line 41-43. No later step re-scans the path for embedded credentials. Result: the recorded telemetry event's url field, stored by AgentTelemetryStore.recordNetworkEvent and shipped to Rollbar, retains the plaintext credential from the nested URL. Base branch has no such feature at all (agent is new), so this is a newly introduced credential-leak path in the primary (successful-parse) branch, distinct from the fallback-path-focused pending finding at UrlSanitizer.java:107.
Verification: nit (security-relevant edge case, newly introduced by this module). The leak is real and reachable on the primary path. For a well-formed URL such as https://gateway.example.com/proxy/https://user:secret@ backend.internal/data, new URI(...) parses successfully (: and @ are legal pchars in path segments, so no URISyntaxException → primary branch, not fallbackSanitize). getRawAuthority()…
| ) { | ||
| try { | ||
| if (thrown != null) { | ||
| if (NetworkEventBridge.markAsRecorded(thrown)) { |
There was a problem hiding this comment.
🟡 (optional) A caller whose app wraps Apache CloseableHttpClient (HC4 or HC5) with its own decorator subclass can get two 'Network error' telemetry events for one failed request, unlike a single event for the response path. DoExecuteAdvice.onExit's thrown branch dedups via NetworkEventBridge.markAsRecorded(thrown) (ApacheHttpClient4Instrumentation.java:161, ApacheHttpClient5Instrumentation.java:161), keyed on throwable identity. The class's own javadoc says outer+inner doExecute both fire for one request and 'still records a single event' - true only because response objects pass through unchanged; if the outer decorator catches the inner IOException and rethrows a different exception (logging/wrapping), both layers see distinct thrown objects and both record. …
Why this was flagged
…Fix: key the error branch on something shared by both layers for one dispatch (e.g. target/request), as already done for the response branch and for HttpUrlConnectionInstrumentation's connection-keyed fix, covering both HC4 and HC5.
Trigger: an application registers a custom CloseableHttpClient subclass that wraps another CloseableHttpClient and overrides doExecute to add logging/retry, catching the inner doExecute's IOException and rethrowing a new exception instance (not the same object). Both the inner and outer doExecute invocations are advised by DoExecuteAdvice (ApacheHttpClient4Instrumentation.java:142-176, and identically in ApacheHttpClient5Instrumentation.java), since both are matched by hasSuperType(CloseableHttpClient). onExit's thrown branch (line 160-166) calls NetworkEventBridge.markAsRecorded(thrown); since the two exception objects are distinct, both markAsRecorded calls return true, so recordError fires twice for one logical failure. The class javadoc explicitly anticipates the wrapping scenario for the response path but the error path's identity-based key was…
Verification: nit. The dedup gap is real. DoExecuteAdvice.onExit keys error dedup on throwable object identity: line 161 if (NetworkEventBridge.markAsRecorded(thrown)), and markAsRecorded (NetworkEventBridge.java:67-69) is RECORDED.add(key) — true only the first time that exact object is seen. The javadoc (lines 148-150) states outer+inner doExecute both fire for a wrapping client and are meant to record…
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether the new per-application partitioning in AgentTelemetryStore regressed the old single global 100-event cap — it didn't; the per-classloader cap (add(), line 275) is a deliberate replacement matching the updated per-app buffering design, not a dropped safeguard.
Extended reasoning...
This run's inline findings concern AgentTelemetryStore's new per-application event partitioning (registerApplication/visibleTo/EVENTS), which handles cross-application telemetry isolation in shared-JVM deployments — a data-exposure-relevant area given the PR's own history of a prior cross-app-leakage finding this partitioning was added to fix. I independently read add() and getAll() and confirmed the per-classloader 100-event cap is an intentional replacement of the old global cap, not a regression, so I ruled that specific candidate out; the two CONFIRMED findings about incomplete partitioning coverage stand as inline comments. Given an exit reason of max_bugs (not dry_streak) and confirmed findings in security/data-isolation-relevant code, approval is not warranted this run.
This pull request has been reviewed before and this review found new issues. Where they share a root cause, one fix may close them together.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (11):
- Unresolved: 11 minor or pre-existing.
| private static void add(Map<String, String> event) { | ||
| ClassLoader origin = contextClassLoader(); | ||
| synchronized (LOCK) { | ||
| Deque<Map<String, String>> buffer = EVENTS.get(origin); | ||
| if (buffer == null) { | ||
| buffer = new ArrayDeque<>(); | ||
| EVENTS.put(origin, buffer); | ||
| } | ||
| if (buffer.size() >= MAX_EVENTS) { | ||
| buffer.pollFirst(); | ||
| } | ||
| buffer.addLast(event); |
There was a problem hiding this comment.
🟡 (optional) The 100-event cap is enforced per origin ClassLoader (add(), line 275), not per application as README.md:150/164 and the MAX_EVENTS javadoc promise. An app that makes HTTP calls from several classloaders the store treats as "nested" (JSP reload loaders, plugin/sandbox loaders, per-module executors) gets one independent 100-slot buffer per classloader, so getAll() can return far more than 100 raw agent events for one deployment. Fix: enforce MAX_EVENTS across all buffers that map to the same visible application (e.g. cap on read/aggregation in getAll, or key buffers by the registered application rather than the raw origin classloader), not per raw ClassLoader key.
Why this was flagged
Trigger: any app whose plugin/JSP/sandbox subsystem runs HTTP calls under several distinct nested ClassLoaders concurrently, all visible to the same registered application via AgentTelemetryStore.visibleTo (AgentTelemetryStore.java:187-196). add() (line 267-280) keys EVENTS by contextClassLoader() and caps each key's Deque at MAX_EVENTS=100 (line 275) independently. getAll(ClassLoader) then unions every visible buffer with no combined cap, so a deployment with N live nested classloaders can accumulate up to N*100 buffered events instead of the documented 100 per application (README.md:150,164; AgentTelemetryStore.java:74-77 javadoc). This inflates memory retained by the store and the work getAll() does merging/sorting, beyond what the docs describe; final report size is still re-capped by AgentTelemetryEventTracker's merge, so the effect is resource growth rather than an oversized payload.
Verification: Severity: nit. Mechanism is real: add() keys the buffer by the thread's context classloader (AgentTelemetryStore.java:268-270, EVENTS.get(origin)) and caps each key's Deque independently (lines 275-276, if (buffer.size() >= MAX_EVENTS) buffer.pollFirst()). getAll(ClassLoader) then unions every buffer visibleTo the requester with no combined cap (lines 174-181), and visibleTo deliberately…
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the newly reported inline finding, two other candidate issues were checked and ruled out this run: AgentTelemetryStore.getAll(ClassLoader) does not NPE when a WeakHashMap entry is GC'd mid-iteration (WeakHashMap's iterator safely skips cleared entries rather than exposing a null), and NetworkEventBridge's async-callback stack walk is a per-request-creation cost rather than a correctness bug.
Extended reasoning...
This is a large, long-running PR adding a new bytebuddy-based Java agent module (HTTP instrumentation for HttpURLConnection, java.net.http, Apache HC4/5) plus a core-SDK telemetry tracker; the security-relevant surface is URL sanitization/credential stripping and cross-application data isolation in the shared telemetry buffer. A CONFIRMED inline finding exists this run (classloader-lookup gap in AgentTelemetryStore's per-application partitioning), a prior CHANGES_REQUESTED review from a human reviewer has at least one thread with no recorded resolution, and there is a long history of findings resolved only by the author or by this bot's own session (not independent), so this stays a defer rather than an approval.
Still open from earlier reviews (12):
- Unresolved: 12 minor or pre-existing.
| static ClassLoader currentOrigin() { | ||
| ClassLoader context = orSystem(Thread.currentThread().getContextClassLoader()); | ||
| ClassLoader caller = callingApplication(); | ||
| if (caller == null || caller == context) { | ||
| return context; | ||
| } | ||
| return isNestedIn(caller, context) ? caller : context; | ||
| } | ||
|
|
||
| private static ClassLoader callingApplication() { | ||
| try { | ||
| return StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE) | ||
| .walk(frames -> frames | ||
| .map(StackWalker.StackFrame::getDeclaringClass) | ||
| .map(Class::getClassLoader) | ||
| .filter(AgentTelemetryStore::isApplicationClassLoader) | ||
| .findFirst() | ||
| .orElse(null)); | ||
| } catch (Throwable ignored) { | ||
| // A SecurityManager can refuse the walk, and an agent must never break the call it observes. | ||
| // The context classloader still answers. | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| // Everything the JDK and the agent itself are loaded by is infrastructure, not an application. | ||
| // In a plain `java -cp app.jar` deployment the application is loaded by the agent's own | ||
| // classloader too, and is correctly left to the context classloader to identify. | ||
| private static boolean isApplicationClassLoader(ClassLoader loader) { | ||
| return loader != null && loader != AGENT_LOADER && loader != PLATFORM_LOADER; | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Apps that load Apache HttpClient (HC4/HC5) from a classloader other than their own — e.g. a container's shared 'common lib' dir rather than WEB-INF/lib — can have their network telemetry silently dropped instead of shown in that app's Rollbar report. callingApplication() (AgentTelemetryStore.java:308-322) returns the first stack frame isApplicationClassLoader (line 327-329) doesn't exclude, and that filter only excludes AGENT_LOADER/PLATFORM_LOADER, so it stops at doExecute()'s own class (defined by the shared library classloader) instead of the real app frame further up the stack. …
Why this was flagged
…If the thread's context classloader also resolves to that shared loader (a background pool, or a thread not yet scoped to the request), currentOrigin() files the event there, and no registered app's getAll() can see it. Fix: also skip classloaders that are strict ancestors of every registered application (or of the instrumented HTTP-client packages), not only the two hardcoded infrastructure loaders.
Trigger: Apache HttpClient (HC4/HC5) is loaded by a classloader other than the calling app's own (e.g. a container's shared 'common' lib dir, a standard pattern for shared enterprise libraries), and the call runs on a thread whose context classloader isn't scoped to that app (a background pool, or a thread not yet given the per-request TCCL). Entry: ApacheHttpClient4Instrumentation.DoExecuteAdvice.onExit (ApacheHttpClient4Instrumentation.java:152-175) is inlined into doExecute(), so its stack frame belongs to the concrete CloseableHttpClient class defined by the shared classloader; it calls recordResponse -> NetworkEventBridge.recordNetworkEvent -> AgentTelemetryStore.currentOrigin() (line…
Verification: Severity: nit (narrow config; loss of telemetry, not a crash/leak — base has no such module so nothing that works on base regresses). The mechanism is real and reachable. callingApplication() (AgentTelemetryStore.java:308-316) returns the FIRST stack frame whose classloader passes isApplicationClassLoader (327-329), which excludes only AGENT_LOADER/PLATFORM_LOADER/null. Apache HC4's…
4 participants
Description of the change
Add Java agent for automatic network telemetry capture
Auto-instruments all major HTTP clients via
-javaagent:using ByteBuddy, capturing 4xx/5xx responses as Rollbar telemetry events with no changes at HTTP call sites.Scope of "no code changes": request code is never touched — no wrappers, no interceptors, no per-call bookkeeping, and nothing to remember when a new HTTP call is added. Setup is a one-time wiring step: the agent JAR on the application classpath, and
.telemetryEventTracker(RollbarAgent.getTelemetryTracker())on the config builder.That wiring is not automatic by design of the current SDK:
ConfigBuilder.build()installs its defaultRollbarTelemetryEventTrackerwhenevertelemetryEventTracker(...)was not called, and there is no global registry orServiceLoaderhook an agent could claim instead. Making the agent self-installing would require a change torollbar-javaand is tracked separately.Caution
This module targets JVM-based applications only. Android is not supported — ART does not implement
the
java.lang.instrumentAPI required by Java agents. Android users should use the existingrollbar-androidmodule instead.The acceptance criteria on the Shortcut story need the same narrowing — "zero application code changes" → "no changes at HTTP call sites; one-time tracker wiring at init".
What's included:
Usage:
Type of change
Related issues
Checklists
Development
Code review