Do not use null parent context on tomcat advice#11968
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e10a139f87
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e10a139 to
0ee35f5
Compare
|
@codex review |
There was a problem hiding this comment.
More details
The PR correctly guards against null parentContext when accessing inferred proxy span operations. The original code would NPE when parentContext was null because InferredProxySpan.fromContext(parentContext) calls .get() on a null object. The fix wraps the two affected operations in an if (parentContext != null) check and handles the null case in the return statement by returning the span directly (AgentSpan extends Context, so it's type-safe). This is a focused defensive programming fix that resolves a real production crash with no unintended side effects.
📊 Validated against 3 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 0ee35f5 · What is Autotest? · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ee35f5166
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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. |
jandro996
left a comment
There was a problem hiding this comment.
The fix itself looks solid (defense in depth: normalizing parentContext to root() at the top of startSpan, plus a null-tolerant fromContext). One request before merging: add a regression test.
PerfectSlayer
left a comment
There was a problem hiding this comment.
I don't think this is the right wait to fix the issue. Added a comment about it
be05cbf to
bd8e1cd
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
jandro996
left a comment
There was a problem hiding this comment.
Good catch on the real root cause, much cleaner fix. Since this was a production NPE, could you add a small regression test for it? Even a unit test on ServiceAdvice.onService with a non-Context attribute would do.
PerfectSlayer
left a comment
There was a problem hiding this comment.
Thanks for the follow up change
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
HttpServerDecorator is doing a couple of actions concerning the inferred proxy spans that assumes that the parent context used to extract this information is not null but actually the tomcat 5.5 instrumentation passes a null context
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]