Conversation
ygree
added
tag: ai generated
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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 739dc55b78
ℹ️ 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".
| final request = new CustomRequest(request: delegate) | ||
|
|
||
| when: | ||
| final result = request.getSession() |
There was a problem hiding this comment.
This instrumented call cannot detect the regression the deleted direct-advice test covered: GetHttpSessionAdvice uses @Advice.OnMethodExit(suppress = Throwable.class), so if the null guard is removed and the context store again throws on get(null), Byte Buddy suppresses that exception and this test still receives the session, observes getServletContext(), and sees no module calls. The equivalent javax test at JettyServlet3Test.groovy:583 has the same blind spot; retain a direct advice invocation or otherwise assert that the advice body completes past the potentially throwing context-store access.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That reasoning overlooks the test harness: suppressed advice exceptions are recorded in InstrumentationErrors, and InstrumentationSpecification checks for those errors during cleanup. Removing the guard therefore fails the replacement tests even if the session is returned and the module remains untouched.
Starting test: getSession tolerates a null servlet context from JakartaHttpServletRequestInstrumentationTest
09:18:52.859 [Test worker] DEBUG datadog.trace.bootstrap.ExceptionLogger - Failed to handle exception in instrumentation for JakartaHttpServletRequestInstrumentationTest$CustomRequest - datadog.trace.instrumentation.servlet5.IastOptOutJakartaHttpServletRequestInstrumentation$GetHttpSessionAdvice
java.lang.NullPointerException: null
at com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap.get(WeakConcurrentMap.java:109)
at datadog.trace.agent.tooling.WeakMaps$Adapter.get(WeakMaps.java:69)
at datadog.trace.bootstrap.WeakMapPerStore.get(WeakMapPerStore.java:33)
at datadog.trace.bootstrap.FieldBackedContextStore.get(FieldBackedContextStore.java:26)
at JakartaHttpServletRequestInstrumentationTest$CustomRequest.getSession(JakartaHttpServletRequestInstrumentationTest.groovy:515)
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The replacement servlet 3/5 tests still return the session if null-context advice throws because Byte Buddy suppresses advice exceptions. Deleting the direct advice tests therefore removes the only assertion that the null guard itself does not fail.
🤖 Bits Code Review · Commit 739dc55 · @DataDog review to ask questions
| then: | ||
| result.is(session) | ||
| 1 * delegate.getSession() >> session | ||
| 1 * delegate.getServletContext() >> null |
There was a problem hiding this comment.
If the null-context guard regresses, the context-store access throws, but @Advice.OnMethodExit(suppress = Throwable.class) swallows that failure. The request still returns the session and the module remains untouched, so both replacement tests pass despite the regression; deleting the direct advice tests therefore removes the effective check that the guard prevents an instrumentation error.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
There was a problem hiding this comment.
That reasoning overlooks the test harness: suppressed advice exceptions are recorded in InstrumentationErrors, and InstrumentationSpecification checks for those errors during cleanup. Removing the guard therefore fails the replacement tests even if the session is returned and the module remains untouched.
Starting test: getSession tolerates a null servlet context from IastJettyServlet3ForkedTest
09:22:25.972 [Test worker] DEBUG datadog.trace.bootstrap.ExceptionLogger - Failed to handle exception in instrumentation for IastJettyServlet3ForkedTest$CustomRequest - datadog.trace.instrumentation.servlet3.IastOptOutHttpServletRequest3Instrumentation$GetHttpSessionAdvice
java.lang.NullPointerException: null
at com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap.get(WeakConcurrentMap.java:109)
at datadog.trace.agent.tooling.WeakMaps$Adapter.get(WeakMaps.java:69)
at datadog.trace.bootstrap.WeakMapPerStore.get(WeakMapPerStore.java:33)
at datadog.trace.bootstrap.FieldBackedContextStore.get(FieldBackedContextStore.java:26)
at IastJettyServlet3ForkedTest$CustomRequest.getSession(JettyServlet3Test.groovy:596)
This branch has not been deployed
1 participant
What Does This Do
Replace direct advice calls in #12692 with calls to getSession() on instrumented requests that return a null servlet context. Verify that the session is returned and no instrumentation errors occur.
Motivation
Exercise the same path used at runtime, verifying that the null guard works when the agent applies the advice to a servlet request.
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]