skill(apm-integrations): Message Queues - document per-item span pattern for batch-consume ops - #12293
skill(apm-integrations): Message Queues - document per-item span pattern for batch-consume ops#12293jordan-wong wants to merge 5 commits into
Conversation
…sume operations When a client API returns a batch of items from one call (e.g. a message broker's poll returning N records), spanning the batch call itself prevents attaching per-item follow-on work to the item that triggered it. Document the wrap-the-iterable pattern already used by kafka-clients (TracingIterable/ TracingIterator/TracingList/TracingListIterator) as the standard for any future batch-consume instrumentation, not just messaging libraries.
🟢 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. |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
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: 50ff311c27
ℹ️ 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".
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The guidance can remove required client-operation spans from search and bulk integrations. It also gives incorrect timeout behavior for abandoned root iterations in the default legacy context manager.
🤖 Datadog Autotest · Commit 50ff311 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
jordan-wong
changed the title
…er covers a callback overload too If a single advice's method matcher covers both a synchronous overload and a sibling overload that accepts a completion callback, unconditionally finishing the span in the exit advice truncates the callback-based call's span duration and drops any error the callback would report — the span finishes when the submitting call returns, not when the operation actually completes. Document the fix: branch on whether the callback argument is present, and finish the span from the wrapped callback instead of the exit advice for that call.
This comment has been minimized.
This comment has been minimized.
…idance Addresses 4 review findings on the "one span per item" rule: - Scope it to independent-consume domains (message/job queues) only. Search and bulk client operations return many elements from ONE outbound call and must stay a single span (see Elasticsearch7RestClientInstrumentation) -- applying the per-item pattern there would misattribute application work and produce thousands of spans for one request. - Add context extraction to the example: each item's span must be started from that item's own propagated context (e.g. its headers), not the caller's active context -- items in a batch can come from different producers. The prior example omitted this entirely. - Correct the abandoned-iteration claim: under the default legacy context manager, activateNext's root iteration scopes ARE force-finished by a background cleaner after trace.scope.iteration.keep.alive (default 30s) -- the prior text said no such timeout exists. - Note that a List-implementing wrapper (mirroring TracingList) does not preserve equals/hashCode -- callers relying on List equality need a different wrapper shape or explicit overrides.
jordan-wong
added
inst: kafka
…s doc-only change)
…age (robolectric-4.17/openai-java 4.59-4.69), now resolved on master
This branch has not been deployed
1 participant
What Does This Do
Adds two sections to
.agents/skills/apm-integrations/references/advice-class.md:Iterable/Iterator/Listso each item gets its own span — opened onnext(), closed when the following item starts or when iteration ends — rather than spanning the batch-returning call itself.send(Message)andsend(Message, CompletionListener)), unconditionally finishing the span in the exit advice truncates the callback-based call's span duration and drops any error the callback would report. The advice must branch on whether the callback is present and finish the span from the wrapped callback instead, for that call.Motivation
Both patterns were found via a blind regeneration research cycle on the messaging category (JMS, kafka-clients) and confirmed against the current codebase before encoding:
kafka-clients-0.11(TracingIterable/TracingIterator/TracingList/TracingListIterator), but had never been written down — without it, a future messaging (or other batch-returning) instrumentation has no way to discover the pattern except by happening to copy kafka's code.send(Message)andsend(Message, CompletionListener)were matched by one advice that always finished the span at exit, truncating the async variant's span and dropping its errors.Additional Notes
Off-band engineering guidance, not tied to a specific PR review comment. No code changes — skill/reference documentation only.
Note: two other candidate findings from the same regen cycle (a
messaging.system/messaging.operationtag requirement, and asuper(...)naming-collision rule) were investigated and NOT added here — the tag claim didn't hold up against master (no dd-trace-java messaging integration sets those tags; they're OTel-shim-only conventions), and the naming rule is already fully covered byinstrumenter-module.md's existing version-alias guidance (the gap there is adherence, not missing content).Jira ticket: [none]