Prevent NPE in HttpClientDecorator.isAgentRequest for a null request - #12376
Prevent NPE in HttpClientDecorator.isAgentRequest for a null request#12376dougqh wants to merge 2 commits into
Conversation
ByteBuddy advice runs before the instrumented method's own argument validation, so a caller passing a null request (e.g. httpClient.sendAsync(null, ...)) NPEs inside isAgentRequest -> getRequestHeader before the JDK's own null-check gets a chance to fire, producing a spurious "Failed to handle exception in instrumentation" telemetry entry instead of the real NPE the JDK would have thrown. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
More details
The null guard stops advice from reading headers from a null request. It preserves the instrumented method's own argument validation.
🤖 Datadog Autotest · Commit 49b13a5 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 310723a: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
What Does This Do
Guards
HttpClientDecorator.isAgentRequest(REQUEST)against anullrequest, returningfalseinstead of throwing anNPEfrom insidegetRequestHeader. Adds a JUnit 5 regression test.Motivation
ByteBuddy method advice runs before the instrumented method's own argument validation. So when a caller passes a null
HttpRequest(e.g.httpClient.sendAsync(null, bodyHandler)), ourSendAsyncAdvice/SendAdvicefor thejava.net.httpinstrumentation dereferences it first viaisAgentRequest(request)→getRequestHeader(request, ...)and NPEs — before the JDK's own null-check on that same argument ever gets a chance to fire its own, correctly-attributed NPE.The advice's
suppress = Throwable.classkeeps this from breaking the caller (the real method still runs afterward and throws its own NPE as expected), but the swallowed exception is logged/recorded as an instrumentation failure ("Failed to handle exception in instrumentation for ... - SendAsyncAdvice"), which is what surfaced this NPE misattributed as an instrumentation bug in Datadog error tracking (issue83642586-aab1-11f0-a63b-da7ad0900002).Fixing in the shared
HttpClientDecoratorbase class rather than only inJavaNetClientDecoratorso every HTTP client integration built on it is protected the same way.Additional Notes
techdebtandperf-reviewskills were run over this branch: no findings from either —isAgentRequestis on the per-request hot path, but the added guard is a single cheap null check with no allocation.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueJira ticket: N/A