From 50ff311c2719dfb5c5362bd991645dc4ffcbba9b Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Tue, 25 Aug 2026 14:13:54 -0400 Subject: [PATCH 1/5] skill(apm-integrations): document 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), 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. --- .../references/advice-class.md | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index ad140b8d94e..b1144d8284c 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -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. + +```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 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 records) { + if (records != null) { + records = new TracingIterable(records, DECORATE.operationName(), DECORATE); + } +} +``` + +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` or `List`) — 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). + ### 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. From 708355a7ce615ba366b0b767abf17a6f8aafd25a Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Wed, 9 Sep 2026 10:27:24 -0400 Subject: [PATCH 2/5] skill(apm-integrations): don't finish the span at exit when one matcher covers a callback overload too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../references/advice-class.md | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index b1144d8284c..218a29b2924 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -134,6 +134,42 @@ Before adding advice to an async wrapper, trace the call path to the sync delega The completion callback advice (whenComplete-style) is still useful for span-close cleanup on the caller's future, but it does not by itself guarantee the sync client's advice sees the right parent. See `context-tracking.md` for the propagation patterns and the specific `readOnly`/lambda constraints. +### One matcher spanning sync and callback-based overloads must not finish the span at method exit + +This is a distinct failure from the double-span case above: here there is only ONE advice, but its method matcher is broad enough to also match an overload that takes a completion callback (e.g. a JMS-style `send(Message)` and `send(Message, CompletionListener)`, or any library's `doThing(Args)` / `doThing(Args, Callback)` pair). If the exit advice unconditionally finishes the span, the callback-based overload's span is finished when the submitting call returns — not when the operation actually completes — truncating its duration and losing any error the callback would have reported. + +**Fix:** either (a) split into two advices with matchers narrow enough to distinguish the callback-taking overload from the synchronous one, or (b) in a single advice, branch on whether the callback parameter is present: if absent, finish the span at exit as normal; if present, wrap the callback (mirroring the delegate-wrapper pattern used for async HTTP clients above) so the wrapper's `onCompletion()`/`onSuccess()`/`onError()` finishes the span, and do NOT finish it in the exit advice for that call. + +```java +// WRONG — same exit advice finishes the span whether or not a callback was supplied +@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) +public static void exit(@Advice.Enter AgentScope scope, @Advice.Thrown Throwable thrown) { + if (scope != null) { + DECORATE.onError(scope.span(), thrown); + scope.close(); + scope.span().finish(); // wrong for the callback-based overload — completes too early + } +} + +// CORRECT — the callback-based overload's advice wraps the callback and does NOT +// finish the span itself; only the callback-less overload's advice finishes at exit +@Advice.OnMethodExit(suppress = Throwable.class) +public static void exit( + @Advice.Enter AgentScope scope, + @Advice.Argument(value = 1, readOnly = false) CompletionListener listener) { + if (scope != null) { + if (listener != null) { + listener = new DatadogCompletionListener(listener, scope.span()); + } else { + scope.span().finish(); + } + scope.close(); + } +} +``` + +**How to discover**: before writing the exit advice, check whether the method being matched has a sibling overload that accepts a callback/listener parameter. If the matcher (or the matched method set) covers both, this rule applies. + ## Multiple advice classes and `@AppliesOn` If your instrumentation needs to apply multiple advices to the same method (e.g. separate context-tracking from tracing logic), use `applyAdvices()` inside `methodAdvice()`. Use the `@AppliesOn` annotation to control which target systems each advice applies to. From 195b0fb159f9e9553f7dbc098cc7f4c73e4b71be Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Tue, 22 Sep 2026 08:49:00 -0400 Subject: [PATCH 3/5] skill(apm-integrations): fix review findings on batch-consume span guidance 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. --- .../references/advice-class.md | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index 218a29b2924..68cf2e0657e 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -33,9 +33,9 @@ Exit method: ### 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. +**Scope: independent-consume domains only** — a message broker's poll returning N records, a job queue's bulk-dequeue returning N jobs, or any API where each returned item carries its own distributed-trace context and independent follow-on work (deserializing, dispatching to a handler, downstream calls). This does NOT apply to a single outbound client operation that happens to return many elements — a search client returning a page of hits, or a bulk API's aggregate result — those are ONE client operation and get ONE span (see `Elasticsearch7RestClientInstrumentation`'s single span around `performRequest`/`performRequestAsync` for the canonical shape); wrapping their result to emit one span per element misattributes application work to spans that don't represent independent operations and can produce thousands of spans for one request. -**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. +**The pattern**: for domains in scope, 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. Extract each item's own distributed-trace context (not the caller's active context) before starting its span — items in the same batch can carry different propagation headers from different producers. ```java // WRONG — one span covers the whole batch; no way to attach per-item follow-on work @@ -45,19 +45,30 @@ public static void exit(@Advice.Return Iterable records) { // ... 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) +// WRONG — per-item spans, but started under the caller's active context instead of +// each item's own propagated context; misparents (or unparents) every record's trace @Advice.OnMethodExit(suppress = Throwable.class) public static void exit(@Advice.Return(readOnly = false) Iterable records) { if (records != null) { records = new TracingIterable(records, DECORATE.operationName(), DECORATE); + // TracingIterable/TracingIterator must extract context per item — see CORRECT below } } + +// CORRECT — wrap the Iterable so each item gets its own span, extracted from that +// item's own propagated context, started on next(), and closed when the following +// item starts (or when iteration ends). This mirrors kafka-clients' TracingIterator: +// AgentSpanContext ctx = extractContextAndGetSpanContext(item.headers(), GETTER); +// AgentSpan span = startSpan(DECORATE.operationName(), ctx); ``` -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. +The wrapping iterator's `next()` starts the span for the item it returns — after extracting that item's own propagated context (e.g. from its headers), not reusing whatever context is currently active — and 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 not closed by `hasNext()`/`next()` — but it is NOT permanently leaked either under the default (legacy) context manager: root iteration scopes opened via `activateNext` are force-finished by a background cleaner after `trace.scope.iteration.keep.alive` (default 30 seconds) — see `IterationSpansForkedTest`. Don't assume this keep-alive exists under a non-legacy context manager without checking; state the timeout behavior you're relying on rather than assuming either "never" or "always" cleans up. + +**If the batch is returned as a `List`** and you wrap it with a `List`-implementing delegator (mirroring `TracingList`), be aware the reference implementation does not override `equals`/`hashCode` — wrapped and unwrapped lists with identical contents will not necessarily compare equal, and equality can be asymmetric depending on operand order. If your integration's tests or downstream code rely on `List` equality, wrap a narrower type (`Iterable`/`Iterator`) instead, or add `equals`/`hashCode` overrides that delegate to the wrapped list's contents. -**How to discover the right hook point**: don't span the accessor that *returns* the batch (e.g. a `records()`/`poll()` method returning `Iterable` or `List`) — 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). +**How to discover the right hook point**: first confirm the API is in scope (independent-consume, not a single client operation returning many elements — see above). If it is, don't span the accessor that *returns* the batch (e.g. a `records()`/`poll()` method returning `Iterable` or `List`) — 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 other independent-consume batch APIs (message queues, job queues), not for client operations that happen to return a collection. ### onExit handling when the target method throws From 9e03296ae2fa6f1841350c210ae033f73a9d076c Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Wed, 23 Sep 2026 09:24:37 -0400 Subject: [PATCH 4/5] chore: retrigger CI (muzzle/test_inst shard failures unrelated to this doc-only change) From daec7459ff7f61dbfe2238500104b69ba251615a Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Wed, 23 Sep 2026 21:21:35 -0400 Subject: [PATCH 5/5] =?UTF-8?q?chore:=20retrigger=20CI=20(2nd)=20=E2=80=94?= =?UTF-8?q?=20confirmed=20transient=20upstream=20muzzle=20breakage=20(robo?= =?UTF-8?q?lectric-4.17/openai-java=204.59-4.69),=20now=20resolved=20on=20?= =?UTF-8?q?master?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit