[Feat] [SDK-399] Add java agent for network telemetry events#374
[Feat] [SDK-399] Add java agent for network telemetry events#374buongarzoni wants to merge 28 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.
…rom overwriting it
…es and @-in-path URLs
| public static String composeUrl(String baseUri, String requestUri) { | ||
| if (requestUri == null || requestUri.isEmpty()) { | ||
| return baseUri != null ? baseUri : ""; | ||
| } | ||
| if (requestUri.contains("://")) { | ||
| return requestUri; | ||
| } | ||
| if (baseUri == null) { | ||
| return requestUri; | ||
| } | ||
| if (requestUri.startsWith("/")) { | ||
| return baseUri.concat(requestUri); | ||
| } | ||
| return baseUri.concat("/").concat(requestUri); | ||
| } |
There was a problem hiding this comment.
🟡 The contains("://") check at NetworkEventBridge.java:145 also matches when :// appears inside a query parameter or path, so a relative request URI carrying a nested URL — e.g. /api/redirect?url=https://other.example.com/foo from an OAuth redirect endpoint, URL shortener, or proxy-style API — is misidentified as absolute, the base host is dropped, and after UrlSanitizer strips the query the recorded telemetry URL becomes just /api/redirect with no host. Narrow trigger (HC4/HC5 host-based execute(HttpHost, request) overload + unencoded URL in query + 4xx/5xx), silent data loss rather than a crash, defeats the very purpose composeUrl was added for on that dispatch path. Fix by bounding the :// check to characters before the first /, ?, or # — or equivalently, skip it when requestUri starts with /.
Extended reasoning...
What goes wrong
composeUrl uses requestUri.contains("://") (line 145) as its absolute-vs-relative discriminator. :// is a valid substring of a query value or a path segment though, so any relative request URI that carries a nested URL — a common pattern for OAuth redirect endpoints, URL shorteners, callback URLs, and proxy-style APIs — is misidentified as absolute. The base URI is discarded and only the requestUri is passed through to UrlSanitizer, which then strips the query (correctly) and leaves a hostless path.
Step-by-step proof (HC4 host-based execute)
HttpHost target = new HttpHost("localhost", 8080, "http");
BasicHttpRequest req = new BasicHttpRequest("GET",
"/api/redirect?url=https://other.example.com/foo");
client.execute(target, req); // server returns 500DoExecuteAdvice.onExitfires;target.toURI()="http://localhost:8080",request.getRequestLine().getUri()="/api/redirect?url=https://other.example.com/foo".composeUrlat line 145:requestUri.contains("://")→ TRUE (matches inside the query value).- Returns
requestUriunchanged, discarding the base — the intermediate URL is"/api/redirect?url=https://other.example.com/foo". UrlSanitizer.sanitizeparses this:new URI("/api/redirect?url=...")succeeds with scheme=null, authority=null, path=/api/redirect.- Reconstruction
new URI(null, null, "/api/redirect", null, null).toString()="/api/redirect". - Recorded telemetry URL =
/api/redirect— thelocalhost:8080host is silently lost.
Why existing code does not prevent it
- The check is a plain unbounded
contains; nothing constrains the match to the scheme position. UrlSanitizerdoes not know a base ever existed — by the time it sees the path-only URI, the join decision has already been made.- No test in
NetworkEventBridgeTestexercises a relative URI carrying://in its query (composeUrl_leavesAbsoluteRequestUriUntouchedonly covers a truly absolute URI).
Impact
Silent data loss on the telemetry URL — precisely the field the README calls out as identifying which dependency failed. Requires the intersection of (a) HC4/HC5 host-based execute(HttpHost, request[, context]), (b) a request URI containing an unencoded nested URL in its query or path, and (c) a 4xx/5xx response. The pattern is realistic for OAuth authorize/callback URLs, URL shorteners, share/embed APIs, and any proxy that forwards a target URL as a query parameter. The trigger is narrow and the failure mode is degraded telemetry rather than a crash, so this is a nit — but this dispatch path is exactly the one composeUrl was added to serve, and the check quietly undoes that on realistic inputs.
How to fix
A one-liner: only treat the :// as a scheme delimiter when it appears before any /, ?, or #. Or simpler: if the URI starts with /, treat it as relative regardless.
if (requestUri.startsWith("/")) {
// relative — fall through to the join below
} else {
int sep = requestUri.indexOf("://");
int firstDelim = -1;
for (int i = 0; i < requestUri.length(); i++) {
char c = requestUri.charAt(i);
if (c == "/".charAt(0) || c == "?".charAt(0) || c == "#".charAt(0)) {
firstDelim = i;
break;
}
}
if (sep > 0 && (firstDelim < 0 || sep < firstDelim)) {
return requestUri;
}
}Worth adding a NetworkEventBridgeTest case with a relative URI that contains :// in its query, plus an HC4/HC5 integration test that asserts the recorded URL still contains the target host in that scenario.
2 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 zero code changes required from the user.
Caution
This module targets JVM-based applications only. Android is not supported — ART does not implement the java.lang.instrument API required by Java agents. Android users should use the existing rollbar-android module instead.
What's included:
Usage:
Type of change
Related issues
Checklists
Development
Code review