-
Notifications
You must be signed in to change notification settings - Fork 357
skill(apm-integrations): document per-item span pattern for batch-consume ops #12293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,34 @@ Exit method: | |
| 6. `scope.close()` | ||
| 7. `span.finish()` | ||
|
|
||
| ### 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. | ||
|
|
||
| **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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
An integration can omit latency and error data for the remote client operation. Assertion details
Was this helpful? React 👍 or 👎 |
||
|
|
||
| ```java | ||
| // WRONG — one span covers the whole batch; no way to attach per-item follow-on work | ||
| @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) | ||
| public static void exit(@Advice.Return Iterable<Record> records) { | ||
| AgentSpan span = startSpan(DECORATE.operationName(), ...); | ||
| // ... consume the whole Iterable under one span — individual item work has no span of its own | ||
| } | ||
|
|
||
| // CORRECT — wrap the Iterable so each item gets its own span, started on next() and | ||
| // closed when the following item starts (or when iteration ends) | ||
| @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With the default legacy context manager, the cited Kafka iterator activates each record span through AGENTS.md reference: AGENTS.md:L44-L44 Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The false cleanup model can cause unnecessary or conflicting cleanup code in future integrations. Assertion details
Was this helpful? React 👍 or 👎 |
||
|
|
||
| **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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a batch API returns a Useful? React with 👍 / 👎. |
||
|
|
||
| ### onExit handling when the target method throws | ||
|
|
||
| The `onThrowable = Throwable.class` attribute on `@Advice.OnMethodExit` controls whether the exit advice fires when the **instrumented target method** throws. You **must** set it explicitly to `Throwable.class` for any exit advice that closes a scope or finishes a span — the default skips exceptional termination, which leaks active scopes when the instrumented method throws. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍 / 👎.