skill(apm-integrations): document per-item span pattern for batch-consume ops - #12293
skill(apm-integrations): document per-item span pattern for batch-consume ops#12293jordan-wong wants to merge 1 commit 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".
|
|
||
| ### Batch-consume operations: one span per item, not one span for the whole batch | ||
|
|
||
| Some client APIs return a batch of items from a single call — a message broker's poll returning N records, a search client returning a page of hits, a bulk API returning multiple results. If the caller iterates the batch and does further per-item work (deserializing, dispatching to a handler, downstream calls), a single span around the whole batch call is wrong: it cannot attach any of that follow-on work to the specific item that triggered it, and it does not reflect where the actual work happens or ends. |
There was a problem hiding this comment.
When this guidance is applied to the explicitly listed search pages or bulk API results, it creates a synthetic active span for every returned element even though those elements are parts of one outbound operation rather than independently received messages carrying separate trace contexts. Existing Elasticsearch instrumentation instead creates one span around performRequest/execute (for example, Elasticsearch7RestClientInstrumentation.java:57-104); wrapping a page containing thousands of hits would therefore produce thousands of misleading spans and attribute arbitrary follow-on application work to them. Limit this canonical pattern to domains such as messaging where each item represents an independent consume operation.
Useful? React with 👍 / 👎.
| } | ||
| ``` | ||
|
|
||
| The wrapping iterator's `next()` starts the span for the item it returns, after first closing whichever span was opened for the previous item. Its `hasNext()` closes the last open span when the delegate has no more items — this is what closes out the final item's span if the caller finishes iterating normally, since there's no explicit "close" call for the last item otherwise. If the caller abandons the iteration partway through (stops calling `next()`/`hasNext()` before reaching the end), the last opened span is left unclosed by this mechanism alone — this is an accepted, known gap (spans opened this way are not finished by a background timeout), not something the advice needs to additionally guard. |
There was a problem hiding this comment.
With the default legacy context manager, the cited Kafka iterator activates each record span through activateNext, whose API explicitly closes root iteration scopes after trace.scope.iteration.keep.alive (30 seconds by default); IterationSpansForkedTest.rootIterationScopeLifecycle verifies that behavior. The parenthetical therefore misstates the canonical lifecycle and may lead future implementations to omit the iteration-scope primitive or devise unnecessary cleanup; document the legacy timeout and any different non-legacy behavior explicitly.
AGENTS.md reference: AGENTS.md:L44-L44
Useful? React with 👍 / 👎.
|
|
||
| The wrapping iterator's `next()` starts the span for the item it returns, after first closing whichever span was opened for the previous item. Its `hasNext()` closes the last open span when the delegate has no more items — this is what closes out the final item's span if the caller finishes iterating normally, since there's no explicit "close" call for the last item otherwise. If the caller abandons the iteration partway through (stops calling `next()`/`hasNext()` before reaching the end), the last opened span is left unclosed by this mechanism alone — this is an accepted, known gap (spans opened this way are not finished by a background timeout), not something the advice needs to additionally guard. | ||
|
|
||
| **How to discover the right hook point**: don't span the accessor that *returns* the batch (e.g. a `records()`/`poll()` method returning `Iterable<T>` or `List<T>`) — span the iteration over it. If the batch is returned as an `Iterable`, wrap the `Iterable` (whose `iterator()` produces a wrapping `Iterator`). If it's returned as a `List`, the same wrapping applies to `List.iterator()`/`listIterator()`. See `dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/{TracingIterable,TracingIterator,TracingList,TracingListIterator}.java` for the canonical implementation — this is the reference pattern for any future batch-consume instrumentation (message queues, but not limited to them). |
There was a problem hiding this comment.
When a batch API returns a List and an integration follows this recommendation, replacing it with the cited Kafka TracingList changes application-visible collection behavior: that class delegates most operations but does not override equals or hashCode, so two content-equal lists can stop comparing equal after instrumentation (and equality can become asymmetric depending on operand order). Either limit this pattern to Iterable/Iterator return types or require a transparent List wrapper that preserves the complete List contract before presenting it as the canonical pattern for future integrations.
Useful? React with 👍 / 👎.
| @Advice.OnMethodExit(suppress = Throwable.class) | ||
| public static void exit(@Advice.Return(readOnly = false) Iterable<Record> records) { | ||
| if (records != null) { | ||
| records = new TracingIterable(records, DECORATE.operationName(), DECORATE); |
There was a problem hiding this comment.
For a messaging batch whose records carry different propagation headers, the shown wrapper API provides only an operation name and decorator, so an implementation following it can start every consumer span from the currently active context rather than from the corresponding producer context. The cited Kafka implementation crucially calls extractContextAndGetSpanContext(val.headers(), GETTER) inside TracingIterator.startNewRecordSpan before startSpan; omitting that requirement disconnects or misparents every per-record trace. Make item-specific context extraction (or the transport's documented batch-parent semantics) part of the required pattern and wrapper contract.
Useful? React with 👍 / 👎.
![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
|
|
||
| Some client APIs return a batch of items from a single call — a message broker's poll returning N records, a search client returning a page of hits, a bulk API returning multiple results. If the caller iterates the batch and does further per-item work (deserializing, dispatching to a handler, downstream calls), a single span around the whole batch call is wrong: it cannot attach any of that follow-on work to the specific item that triggered it, and it does not reflect where the actual work happens or ends. | ||
|
|
||
| **The pattern**: wrap the returned `Iterable`/`Iterator`/`List` so that advancing to the next item closes the previous item's span and opens a new one for the current item. Do not span the method that returns the batch; span the act of consuming each item from it. |
There was a problem hiding this comment.
An integration can omit latency and error data for the remote client operation.
Assertion details
- Input: A future integration follows this guidance for a search or bulk client method that sends a remote request and returns results.
- Expected:
Limit the rule to consumer item or delivery spans. State that a separate client-operation span can cover the remote batch request. - Actual:
The rule says not to span a method that returns a batch. Its examples include search and bulk client calls, which are remote client operations.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
| } | ||
| ``` | ||
|
|
||
| The wrapping iterator's `next()` starts the span for the item it returns, after first closing whichever span was opened for the previous item. Its `hasNext()` closes the last open span when the delegate has no more items — this is what closes out the final item's span if the caller finishes iterating normally, since there's no explicit "close" call for the last item otherwise. If the caller abandons the iteration partway through (stops calling `next()`/`hasNext()` before reaching the end), the last opened span is left unclosed by this mechanism alone — this is an accepted, known gap (spans opened this way are not finished by a background timeout), not something the advice needs to additionally guard. |
There was a problem hiding this comment.
The false cleanup model can cause unnecessary or conflicting cleanup code in future integrations.
Assertion details
- Input: A caller stops iteration after next() while the default legacy context manager is active.
- Expected:
State that the cleaner finishes an overdue root iteration span after the configured keep-alive time, which is 30 seconds by default. - Actual:
The guidance says no background timeout finishes an abandoned span. The default legacy context manager schedules a cleaner for root iteration spans.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
1 participant
What Does This Do
Adds a new section to
.agents/skills/apm-integrations/references/advice-class.mddocumenting the per-item span pattern for batch-consume operations: when a client API returns a batch of items from one call (e.g. a message broker's poll returning N records), the instrumentation should wrap the returnedIterable/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.Motivation
This pattern is already implemented correctly in
kafka-clients-0.11(TracingIterable/TracingIterator/TracingList/TracingListIterator), but it has never been written down as guidance. A naive single-span-per-batch-call approach cannot attach per-item follow-on work (deserialization, handler dispatch, downstream calls) to the item that triggered it, and doesn't reflect where the real work starts/ends. Without written guidance, future messaging (or other batch-returning) instrumentations have no way to discover this pattern except by happening to copy kafka's code — this documents it as the standard going forward, citing the existing kafka-clients classes as the canonical reference implementation.Additional Notes
Off-band engineering guidance, not tied to a specific PR review comment. No code changes — skill/reference documentation only.
Jira ticket: [none]