Fix missing instrumentation for vertx 5.1 responses#11840
Conversation
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![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: ebbd5e1486
ℹ️ 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".
| return new VertxServer(verticle(), routerBasePath(), true) | ||
| } | ||
|
|
||
| def 'test blocking of JSON request body finishes route handler span'() { |
There was a problem hiding this comment.
The repo instructions in /workspace/dd-trace-java/AGENTS.md explicitly say not to write new Groovy/Spock tests and to migrate existing Groovy tests to JUnit 5. This adds a new Spock feature method to the Vert.x 5 test suite, so the new regression coverage should be implemented as JUnit 5 coverage instead of extending the deprecated Groovy suite.
Useful? React with 👍 / 👎.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 0afee37 | Docs | Datadog PR Page | Give us feedback! |
🟡 Java Benchmark SLOs — Performance SLO warning (near threshold)
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. |
vandonr
changed the title
|
|
||
| @Test | ||
| void wrapsApplicationEndHandlerOnVertx51Http1Response() throws Exception { | ||
| assumeTrue(hasVertx51Http1Response(), "Http1ServerResponse exists only in Vert.x 5.1+"); |
There was a problem hiding this comment.
not a huge fan of this, but also I don't think it's a big enough change that we should create a new folder for 5.1 ?
| // AppSec uses BlockingException as control flow after committing a blocking response | ||
| // from advice such as WafPublishingBodyHandler and RoutingContextJsonAdvice. Finish | ||
| // immediately because that abort path may bypass Vert.x response/routing end callbacks. | ||
| finishHandlerSpan(routingContext); |
There was a problem hiding this comment.
This catch sees BlockingException thrown synchronously on the actual.handle() stack. But WafPublishingBodyHandler - wraps the async bodyHandler, so it throws on a later event-loop turn, after handle() has already returned.
Might be more reliable to finish the span at the block-commit site (WafPublishingBodyHandler) instead of here, so the async body-block path is covered too.
There was a problem hiding this comment.
hmm ok. I think there is value in having the fix to instrument the responses in the current release, so that we don't ship with a known bug. I'll take a look at how to better handle the async path in a follow up PR (because the fix is not straightforward I think)
There was a problem hiding this comment.
we need ta way to pass the RoutingContext to WafPublishingBodyHandler to be able to finish spans there
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 1358236102403770214 took longer than expected. The current limit for the base branch 'master' is 120 minutes. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
/merge |
|
View all feedbacks in Devflow UI.
PR already in the queue with status in_progress |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in Use ⏳ Processing |
2 participants
What Does This Do
follow up on #11655
newmoved http/1 & /2 server responses classMotivation
I think the missing instrumentation on HttpServerResponseImpl is what caused the test to flake with a missing span. Not sure why it's not failing all the time though, maybe a race condition with another callback that finishes the span ?
Additional Notes
This time, the issue was not in the test but in the instrumentation it seems
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 issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]