[Feat] [SDK-399] Add java agent for network telemetry events - #374
Open
buongarzoni wants to merge 35 commits into
Open
[Feat] [SDK-399] Add java agent for network telemetry events#374buongarzoni wants to merge 35 commits into
buongarzoni wants to merge 35 commits into
Claude / Claude Code Review
completed
Aug 10, 2026 in 22m 11s
Code review found 1 important issue
Found 5 candidates, confirmed 6. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 5 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | rollbar-java-agent/src/main/java/com/rollbar/agent/instrumentation/ApacheHttpClient4Instrumentation.java:85-91 |
HC4/HC5 advice types params as library classes, breaking in classloader-isolated deployments |
| 🟡 Nit | rollbar-java-agent/src/main/java/com/rollbar/agent/RollbarAgent.java:40-49 |
agentmain dynamic attach silently instruments nothing (missing RedefinitionStrategy) |
| 🟡 Nit | rollbar-java-agent/src/main/java/com/rollbar/agent/instrumentation/ApacheHttpClient4Instrumentation.java:100-117 |
HC4/HC5 doExecute advice records pre-redirect host/URI for redirected 4xx/5xx |
| 🟡 Nit | rollbar-java-agent/src/main/java/com/rollbar/agent/instrumentation/HttpUrlConnectionInstrumentation.java:160-168 |
[quality] Duplicated reflective markAsRecorded+recordError block across 3 advice sites |
Annotations
claude / Claude Code Review
HC4/HC5 advice types params as library classes, breaking in classloader-isolated deployments
DoExecuteAdvice types its @Advice.Argument/@Advice.Return parameters as concrete Apache HttpClient types (HttpHost/HttpRequest/HttpResponse in HC4, the hc5 equivalents in HC5) instead of Object + reflection, unlike HttpUrlConnectionInstrumentation and JavaHttpClientInstrumentation which deliberately avoid this. Because Advice.to(DoExecuteAdvice.class) resolves these parameter types via getDeclaredMethods() in the agent's own classloader, whenever Apache HttpClient is loaded by a classloader the
Check warning on line 49 in rollbar-java-agent/src/main/java/com/rollbar/agent/RollbarAgent.java
claude / Claude Code Review
agentmain dynamic attach silently instruments nothing (missing RedefinitionStrategy)
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
claude / Claude Code Review
HC4/HC5 doExecute advice records pre-redirect host/URI for redirected 4xx/5xx
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
claude / Claude Code Review
[quality] Duplicated reflective markAsRecorded+recordError block across 3 advice sites
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.
Loading