Skip to content

Add RxJava 3 instrumentation#11506

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 22 commits into
masterfrom
andrea.marziali/rxjava3
Jun 26, 2026
Merged

Add RxJava 3 instrumentation#11506
gh-worker-dd-mergequeue-cf854d[bot] merged 22 commits into
masterfrom
andrea.marziali/rxjava3

Conversation

@amarziali

@amarziali amarziali commented May 29, 2026

Copy link
Copy Markdown
Contributor

What Does This Do

We already trace RxJava 1 and 2 but not 3. RxJava 3 moved its types to the io.reactivex.rxjava3.core.* namespace, so the existing rxjava-2.0 instrumentation never matches it. This adds a new rxjava-3.0 module that brings RxJava 3 to parity.

What's in it:

  • New dd-java-agent/instrumentation/rxjava/rxjava-3.0 module. It ports the rxjava-2.0 logic to the RxJava 3 namespace: context capture on the five reactive types (Flowable, Observable, Single, Maybe, Completable) and re-attachment around subscriber callbacks, plus the async result extension that finishes @WithSpan / @Trace spans when the stream completes, errors, or is cancelled.
  • Muzzle has a pass for rxjava3 [3.0.0,) and a fail block that asserts the advice can never match the rxjava2 artifact, so the two versions stay isolated. The module also pulls in rxjava-2.0 as a test runtime dependency to confirm both instrumenters coexist.
  • Registered in the GraalVM native image build time list next to the rxjava2 entry.
  • Tests are JUnit 5 (44 cases): subscription propagation, the core propagation suite, the @WithSpan result extension across all five types, and a Java 8 interop check (fromCompletionStage, fromStream, fromOptional). latestDepTest runs the same suite against the newest published rxjava3.

One thing reviewers should look at closely: this also touches the shared java-concurrent module. While testing delayed chains I found that RxJava 3 added io.reactivex.rxjava3.internal.schedulers.AbstractDirectTask, whose static initializer builds two sentinel FutureTask instances. When that initializer first runs under an active trace (the first delay or timeout hop inside a span), the executor instrumentation captures a continuation on those static singletons that never gets cancelled, so the trace stays pending and is never reported. RxJava 2 has no equivalent class, which is why the byte for byte equivalent rxjava-2.0 code never hit this(correction: 2.0.9+ seem to have it). The fix disables async propagation while that type initializer runs, following the same pattern already used for Reactor's SchedulerTask and WorkerTask. The matcher is an exact class name, so it stays inert unless RxJava 3 is on the classpath.

The Java 8 interop check found no propagation gaps once that fix was in place.

Motivation

Additional Notes

Contributor Checklist

  • Format the title according to the contribution guidelines
  • Assign the type: and (comp: or inst:) labels in addition to any other useful labels
  • Avoid using close, fix, or any linking keywords when referencing an issue
    Use solves instead, and assign the PR milestone to the issue
  • Update the CODEOWNERS file on source file addition, migration, or deletion
  • Update public documentation with any new configuration flags or behaviors
  • Add your completed PR to the merge queue by commenting /merge. You can also:
    • Customize the commit message associated with the merge with /merge --commit-message "..."
    • Remove your PR from the merge queue with /merge -c
    • Skip all merge queue checks with /merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level
    • Get more information in this doc

Jira ticket: [PROJ-IDENT]

@amarziali amarziali added type: feature Enhancements and improvements inst: others All other instrumentations ai-generated labels May 29, 2026
Comment on lines +131 to +133
// FINDING: fromStream(2 elements) emits one child span per element (2 spans here), each a direct
// child of interop-parent. The map() runs synchronously on the subscribing thread under the
// active interop-parent scope, so no async/concurrent instrumentation is involved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the javadoc

@datadog-datadog-prod-us1

This comment has been minimized.

@dd-octo-sts

dd-octo-sts Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 13.91 s 13.91 s [-0.7%; +0.7%] (no difference)
startup:insecure-bank:tracing:Agent 12.89 s 12.94 s [-1.0%; +0.4%] (no difference)
startup:petclinic:appsec:Agent 16.85 s 16.42 s [+1.6%; +3.7%] (significantly worse)
startup:petclinic:iast:Agent 16.40 s 16.81 s [-6.8%; +1.9%] (no difference)
startup:petclinic:profiling:Agent 16.79 s 16.93 s [-1.8%; +0.1%] (no difference)
startup:petclinic:sca:Agent 16.91 s 16.84 s [-0.6%; +1.4%] (no difference)
startup:petclinic:tracing:Agent 15.62 s 15.65 s [-6.0%; +5.6%] (unstable)

Commit: a1843edf · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

@amarziali
amarziali force-pushed the andrea.marziali/rxjava3 branch from e8200eb to cedc6cc Compare June 1, 2026 12:10
@PerfectSlayer PerfectSlayer added tag: ai generated Largely based on code generated by an AI or LLM and removed ai-generated labels Jun 3, 2026
amarziali and others added 18 commits June 22, 2026 13:50
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…izer

RxJava 3 introduced io.reactivex.rxjava3.internal.schedulers.AbstractDirectTask,
whose static initializer constructs FINISHED/DISPOSED sentinel FutureTask
instances. When that initializer first runs under an active trace (e.g. the
first delay/timeout scheduler hop inside a span), the executor instrumentation
captures a ScopeContinuation on those static singletons that is never cancelled,
leaking the pending trace so it is never reported.

Disable async propagation while AbstractDirectTask's type initializer runs,
mirroring the existing reactor.core.scheduler.SchedulerTask/WorkerTask handling.
The matcher is inert unless RxJava 3 is on the classpath. RxJava 2 has no
equivalent class. Restores the delayed-Maybe coverage in RxJava3Test, which
fails without this fix and passes with it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ava3

Two tests were under datadog.trace.instrumentation.rxjava3 while the @Trace-using
tests must live under testdog.* (the agent ignores datadog.* for instrumentation).
Move all four tests to testdog.* for consistency.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ValentinZakharov
ValentinZakharov force-pushed the andrea.marziali/rxjava3 branch from 80ff05f to 97dc359 Compare June 22, 2026 11:56
@ValentinZakharov
ValentinZakharov requested review from a team as code owners June 22, 2026 12:33
@ValentinZakharov
ValentinZakharov requested review from dougqh and vandonr and removed request for a team June 22, 2026 12:33

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: af33b38ec7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

isMethod()
.and(named("subscribe"))
.and(takesArguments(1))
.and(takesArgument(0, named("io.reactivex.rxjava3.core.FlowableSubscriber"))),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wrap Flowable's Subscriber overload as well

When a Flowable is consumed through the Reactive Streams API or with a subscriber that only implements org.reactivestreams.Subscriber, RxJava calls the separate subscribe(Subscriber) overload and applies its own compliance wrapper for non-FlowableSubscriber instances. This advice only matches the FlowableSubscriber overload, so TracingSubscriber is never inserted for those subscribers and async emissions such as delay/observeOn lose the captured parent context. This also falls short of the RxJava 2 instrumentation, which matches org.reactivestreams.Subscriber.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This concern applies to RxJava 2, but not RxJava 3. In RxJava 3, subscribe(Subscriber) always delegates to subscribe(FlowableSubscriber) - either directly or by wrapping in StrictSubscriber, which implements FlowableSubscriber. Both paths converge at the same hook point. Verified against Flowable 3.1.12 bytecode


AgentSpan parent = startSpan("test", "parent");
try (AgentScope scope = activateSpan(parent)) {
Maybe<Connection> connection = Maybe.create(emitter -> emitter.onSuccess(new Connection()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably extend coverage for Completable, SIngle, Observable, and Flowable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, extended tests, thanks


@ParameterizedTest(name = "test WithSpan annotated async method cancelled {0}")
@EnumSource(ReactiveType.class)
void cancelled(ReactiveType type) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not critical, but could also be useful to exercise the never constructors for cancellation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test cancelNever to ensure span properly finishes

@ygree ygree left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks!

@vandonr vandonr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lessgo

@ValentinZakharov
ValentinZakharov added this pull request to the merge queue Jun 26, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Jun 26, 2026

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-06-26 17:26:38 UTC ℹ️ Start processing command /merge


2026-06-26 17:26:44 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in master is approximately 1h (p90).


2026-06-26 18:29:39 UTC ℹ️ MergeQueue: This merge request was merged

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 26, 2026
@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot merged commit 35096e2 into master Jun 26, 2026
583 checks passed
@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot deleted the andrea.marziali/rxjava3 branch June 26, 2026 18:29
@github-actions github-actions Bot added this to the 1.64.0 milestone Jun 26, 2026
Comment on lines +37 to +39
// Async completion / scheduler hops can finish child spans after the local root is written,
// tripping strict trace write ordering checks. Mirror RxJava3Test.
testConfig.strictTraceWrites(false);

@amarziali amarziali Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValentinZakharov this was not supposed to be done at all since it hides an issue . in this case it seems it can be removed (the tests pass without)

gh-worker-dd-mergequeue-cf854d Bot pushed a commit that referenced this pull request Jun 29, 2026
Revert "Add RxJava 3 instrumentation (#11506)"

This reverts commit 35096e2.

Co-authored-by: bruce.bujon <bruce.bujon@datadoghq.com>
TophrC-dd pushed a commit that referenced this pull request Jun 29, 2026
Scaffold rxjava-3.0 instrumentation module

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add rxjava-3.0 Tracing observer/subscriber wrappers

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add rxjava-3.0 type instrumentations, module and async result extension

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Register rxjava3 async result extension for Graal native-image

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add rxjava-3.0 SubscriptionTest

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add rxjava-3.0 core context propagation test

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Fix pending-trace leak from RxJava3 AbstractDirectTask static initializer

RxJava 3 introduced io.reactivex.rxjava3.internal.schedulers.AbstractDirectTask,
whose static initializer constructs FINISHED/DISPOSED sentinel FutureTask
instances. When that initializer first runs under an active trace (e.g. the
first delay/timeout scheduler hop inside a span), the executor instrumentation
captures a ScopeContinuation on those static singletons that is never cancelled,
leaking the pending trace so it is never reported.

Disable async propagation while AbstractDirectTask's type initializer runs,
mirroring the existing reactor.core.scheduler.SchedulerTask/WorkerTask handling.
The matcher is inert unless RxJava 3 is on the classpath. RxJava 2 has no
equivalent class. Restores the delayed-Maybe coverage in RxJava3Test, which
fails without this fix and passes with it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add rxjava-3.0 @WithSpan async result extension test

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add rxjava-3.0 Java 8 interop context-propagation investigation test

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Unify rxjava-3.0 test package under testdog.trace.instrumentation.rxjava3

Two tests were under datadog.trace.instrumentation.rxjava3 while the @Trace-using
tests must live under testdog.* (the agent ignores datadog.* for instrumentation).
Move all four tests to testdog.* for consistency.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

review

Add config json entry

Skip root context capture in Observable and Single CaptureParentSpanAdvice

Add scheduler context propagation and no-spurious-traces tests for Observable and Single

Fix misleading comment in correctParentsFromSubscriptionTime test

Extend cancel test to cover all five reactive types

Ensure publisher-parent span is always finished in cancelUnderTrace

Normalize PropagateParentSpanAdvice guard and add @nonnull to TracingObserver

spotless

spotless

Extend SubscriptionTest to cover all five RxJava 3 reactive types

Add cancelledNever test to cover dispose-path span lifecycle in RxJava3ResultExtensionTest

Co-authored-by: ValentinZakharov <valentin.zakharov@datadoghq.com>
Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
TophrC-dd pushed a commit that referenced this pull request Jun 29, 2026
Revert "Add RxJava 3 instrumentation (#11506)"

This reverts commit 35096e2.

Co-authored-by: bruce.bujon <bruce.bujon@datadoghq.com>
@ValentinZakharov ValentinZakharov added the tag: no release notes Changes to exclude from release notes label Jul 2, 2026
mcculls pushed a commit that referenced this pull request Jul 9, 2026
…ep 4.4

Port reviewer-encoded rules from the toolkit's synced derivative
(`apm-integrations` skill in DataDog/apm-instrumentation-toolkit) back
to the canonical `add-apm-integrations` skill.

The toolkit accumulated 21+ numbered reviewer rules during eval research
(commits flowing through PR #11337 jedis-3.0, PR #11562 sparkjava-2.3,
PR #11717 commons-httpclient, PR #11709 feign, and PR #11506 RxJava 3).
These rules encode real failure modes observed during agent-generated
PRs and the fixes reviewers asked for.

Sections added:

- **Step 4.4** — Library category: span-creating vs context-propagation.
  New axis that routes Cat B libraries (reactive, async, executors,
  futures, actors) into a `context_propagation` codegen path producing
  `InstrumenterModule.ContextTracking` instead of span-creating advice.
  Includes the Flowable subscribe(FlowableSubscriber) overload rule
  (hook the framework-internal overload, not the public wrapper).
- **Step 4.5** — Java naming consistency (module-name conventions).

R-rule placements:

- Step 4:  R32 (dir name must end with version OR allowed suffix)
- Step 5:  R13 (no single-type helper class for CallDepthThreadLocalMap)
           R30 (preserve master's integration name when regenerating)
- Step 7:  R15/R16/R17 (single delegate method, not all overloads)
           R33 (no NullPointerException catches; use null-check guards)
- Step 9:  R14 (test error/exception scenarios + spotless)
           R18 (muzzle excludes incompatible majors)
           R19 (latestDepTestImplementation range matches instrumented)
           R20 (Java tests only; no new .groovy files)
           R28 (compileOnly/testImplementation version split rationale)
           R29 (register names in supported-configurations.json)
           R31 (assertInverse only when declared min is true min)
- Step 4.4: R21-R27 (the Cat A vs Cat B classification + Cat B schema)

This PR pairs with toolkit-side PR DataDog/apm-instrumentation-toolkit#472
which adds the same content to the toolkit's synced derivative copy.
Both copies should stay in sync — this is the canonical home.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>
gh-worker-dd-mergequeue-cf854d Bot pushed a commit that referenced this pull request Jul 9, 2026
…o sub references (#11760)

skill(add-apm-integrations): R13-R33 + Cat B (context-propagation) Step 4.4

Port reviewer-encoded rules from the toolkit's synced derivative
(`apm-integrations` skill in DataDog/apm-instrumentation-toolkit) back
to the canonical `add-apm-integrations` skill.

The toolkit accumulated 21+ numbered reviewer rules during eval research
(commits flowing through PR #11337 jedis-3.0, PR #11562 sparkjava-2.3,
PR #11717 commons-httpclient, PR #11709 feign, and PR #11506 RxJava 3).
These rules encode real failure modes observed during agent-generated
PRs and the fixes reviewers asked for.

Sections added:

- **Step 4.4** — Library category: span-creating vs context-propagation.
  New axis that routes Cat B libraries (reactive, async, executors,
  futures, actors) into a `context_propagation` codegen path producing
  `InstrumenterModule.ContextTracking` instead of span-creating advice.
  Includes the Flowable subscribe(FlowableSubscriber) overload rule
  (hook the framework-internal overload, not the public wrapper).
- **Step 4.5** — Java naming consistency (module-name conventions).

R-rule placements:

- Step 4:  R32 (dir name must end with version OR allowed suffix)
- Step 5:  R13 (no single-type helper class for CallDepthThreadLocalMap)
           R30 (preserve master's integration name when regenerating)
- Step 7:  R15/R16/R17 (single delegate method, not all overloads)
           R33 (no NullPointerException catches; use null-check guards)
- Step 9:  R14 (test error/exception scenarios + spotless)
           R18 (muzzle excludes incompatible majors)
           R19 (latestDepTestImplementation range matches instrumented)
           R20 (Java tests only; no new .groovy files)
           R28 (compileOnly/testImplementation version split rationale)
           R29 (register names in supported-configurations.json)
           R31 (assertInverse only when declared min is true min)
- Step 4.4: R21-R27 (the Cat A vs Cat B classification + Cat B schema)

This PR pairs with toolkit-side PR DataDog/apm-instrumentation-toolkit#472
which adds the same content to the toolkit's synced derivative copy.
Both copies should stay in sync — this is the canonical home.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

skill(add-apm-integrations): clean up R-numbering and sub-step naming

Two cosmetic passes on the rule encoding ported from the toolkit's
synced derivative copy. Substantive content unchanged.

Pass 1 — strip R-numbering. The R13-R33 numbers are toolkit-internal
traceability tags that tie each rule to a specific reviewer comment on a
specific generated-PR review. They are meaningful to the toolkit-side
eval research workflow but meaningless to readers of the canonical
skill, who have no R1-R12 context here. Removes:

- '#### R<NN> — <title>' prefixes (14 sub-section headings)
- Inline '(R<NN>)', '(R<NN>/R<NN>/R<NN>)' parentheticals on existing
  bullets that already convey the rule in their wording
- A stale cross-reference 'see R29 below' rewritten to 'see
  "Register new integration names"'

The toolkit-side copy keeps the R-numbering — it remains the eval-research
home where the traceability matters. This is a one-way port convention.

Pass 2 — renumber half-steps so the decimals make sense. Was:

  Step 4 → Step 4.4 → Step 4.5 → Step 5

implying missing 4.1, 4.2, 4.3. The original numbering was an artifact
of an earlier toolkit-side draft that had a 4.1-4.4 enumeration which
got collapsed. Renumbered to:

  Step 4 → Step 4.1 → Step 4.2 → Step 5

Main integer steps (1-12) unchanged. Step 7.1 (Multiple advice classes
and @AppliesOn) left as-is since its decimal already makes sense
relative to Step 7.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

refactor(skill): extract Category B routing to references/

Move the 62-line "Step 4.1 – Library category" section from SKILL.md
into references/category-b-context-propagation.md. SKILL.md keeps a
5-line stub linking to the reference file — enough context to know
when to read it, not enough to bury the rest of Step 4.

Preserves all content verbatim; no wording changes. Follows the
existing dd-trace-java skill convention of tracking specific
per-skill files (git add -f, matching the precedent set by
.claude/skills/migrate-groovy-to-java/QUALITY_RULES.md).

Part 1 of a refactor to slim the 794-line SKILL.md into a routing
overview (~250 lines target) with topic-oriented reference files.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

refactor(skill): extract naming conventions to references/

Move two related naming rules from SKILL.md into
references/naming-conventions.md:

- Step 4's module-directory-name rule (must end with version or
  "-common"/"-stubs"/"-iast" suffix)
- Step 4.2's Java filename ↔ class-name matching rule (with the
  sanity-check script)

They belong together because both are enforcement rules for names.
SKILL.md keeps short stubs linking to the reference file.

Preserves all content verbatim. Part 2 of the SKILL.md slim.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

refactor(skill): extract InstrumenterModule guidance to references/

Move Step 5's 103-line body from SKILL.md into
references/instrumenter-module.md, covering:

- @autoservice + narrow-interface preferences (ForSingleType >
  ForKnownTypes > ForTypeHierarchy) with the interface-only API JAR
  exception (JMS, JPA, JDBC, etc.)
- 'Must NOT do' — no static constants for one-shot methods
- instrumentationNames() version-qualified alias rule
- No helper class for single-target CallDepthThreadLocalMap
- Preserve master's integration name on regeneration
- Advanced: grouping multiple instrumentations under one module

SKILL.md keeps Step 5 as a 6-line summary + link. Preserves all
content verbatim; no wording changes. Part 3 of the SKILL.md slim.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

refactor(skill): extract Advice class guidance to references/

Move Step 7's 149-line body from SKILL.md into
references/advice-class.md. This was the largest single-step body
in the skill and the highest-risk area to get wrong.

Reference file covers:
- Advice method annotations + parameter kinds
- Span lifecycle (enter/exit order)
- onExit resilience to onEnter throwing
- Explicit charset for byte[] to String
- No NullPointerException catches (SpotBugs enforces)
- Single-delegate-method instrumentation (not all overloads)
- @AppliesOn + multiple advice classes
- 'Must NOT do' list (no loggers, no lambdas, no inline=false, etc.)

SKILL.md keeps Step 7 as a summary + link. Preserves all content
verbatim. Part 4 of the SKILL.md slim.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

refactor(skill): extract tests guidance to references/

Move Step 9's 'Instrumentation test' section + all its sub-rules
(no .groovy files, supported-configurations.json registration,
compileOnly/testImplementation version-split rationale,
prior-version-module inclusion) from SKILL.md into references/tests.md.

Muzzle content stays in place for now — it's a separate concern and
gets its own reference file next.

SKILL.md keeps Step 9.1 as a summary + link. Preserves all content
verbatim. Part 5 of the SKILL.md slim.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

refactor(skill): extract muzzle guidance to references/

Move Step 9's 'Muzzle directives' section + all its sub-rules
(assertInverse gotchas, incompatible-major-version exclusion,
skipVersions for malformed release versions) from SKILL.md into
references/muzzle.md.

SKILL.md keeps Step 9.2 as a summary + link. Preserves all content
verbatim; no wording changes. Part 6 of the SKILL.md slim.

Final state: SKILL.md 794 → ~215 lines, split into 6 topic-oriented
reference files under references/.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

style(skill): add missing blank lines before Step/subsection headings

Cosmetic fixup after the section extractions. Three headings lost
their preceding blank line during the awk-based edits — restoring
them so the rendered Markdown reads cleanly.

No content changes.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

refactor(skill): drop toolkit-internal 'Category A/B' language

The 'Category A' / 'Category B' labels came from toolkit-side research
where they were shorthand for the 'target_kind' Pydantic enum values.
They have no meaning in dd-trace-java on their own — a contributor
reading the skill has no context for what 'Category B' refers to.

Replace with the descriptive terms that already exist in dd-trace-java:

- 'span-creating instrumentation' — extends InstrumenterModule.Tracing
- 'context-tracking instrumentation' — extends InstrumenterModule.ContextTracking
  (matches the class name + TargetSystem.CONTEXT_TRACKING enum)

Changes:

- Rename references/category-b-context-propagation.md → references/context-tracking.md
- Rewrite Step 4.1 stub in SKILL.md to drop Category A/B and 'target_kind'
- Rewrite context-tracking.md body from 'Category B target shape' Pydantic-
  field enumeration to 'What a context-tracking instrumentation captures',
  described in Java terms (boundary type, capture/restore points, wrapper
  class, wrapper methods) instead of toolkit Pydantic field names
- Fix advice-class.md's stray 'context-propagation logic' → 'context-tracking
  logic' to match dd-trace-java's TargetSystem.CONTEXT_TRACKING naming

No substantive guidance changed. Reference still points at rxjava-2.0 as
the canonical example.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

refactor(skill): remove toolkit-workflow language from reference files

Two remaining spots reframed from LLM-agent-workflow perspective to
dd-trace-java human-contributor perspective:

- muzzle.md 'Background' paragraph: 'a typical greenfield generation
  produces...' + 'the agent picks the higher version...' → 'this
  failure mode is common when a module has both a sync and async
  instrumentation class' + 'declaring the higher version as the muzzle
  min...'. Same technical content, no LLM-agent workflow assumption.

- tests.md 'How to discover' step: 'run the sample app' → 'run your
  instrumentation test'. 'Sample app' was ambiguous ('the toolkit's
  sample-app workflow step' vs 'your own test app'); the concrete
  dd-trace-java term is 'instrumentation test'.

No substantive guidance changed. Preserves all rules verbatim.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

fix(skill): address review comments on #11760

Nine fixes from Copilot bot + @mcculls review comments:

SKILL.md
- Step 4 source layout: 'src/test/groovy/ — Spock tests' → 'src/test/java/
  — JUnit 5 tests'. Contradicted Step 9.1's Java-only policy. (Copilot)

references/tests.md
- Rewrite the error-test example: 'List<List<SpanData>> traces = ...' used
  OpenTelemetry's SpanData type (won't compile against dd-trace-java's
  TEST_WRITER, which returns List<List<DDSpan>>). Now uses AgentSpan and
  span.getTag() per mcculls's guidance that AgentSpan is enough for tests.
- Replace 'checkNewGroovyFiles' (unverifiable bot name) with the real
  workflow: 'Enforce Groovy Migration'
  (.github/workflows/enforce-groovy-migration.yaml). Both places.
- Default value in supported-configurations.json: change 'false' to 'true'
  per mcculls — ~83% of typical integrations default to true; 'false' is
  reserved for modules that override defaultEnabled() (OpenTelemetry,
  Hazelcast, sparkjava). Add a note calling out the branching.

references/naming-conventions.md
- Remove gRPCInstrumentation as an example — it doesn't exist in the
  codebase; the gRPC integration uses Grpc* (GrpcClientDecorator etc).
  Reframe the section to acknowledge acronym casing is not uniform
  across dd-trace-java and to defer to a reference instrumentation.
  (Copilot)
- Drop the sanity-check bash script entirely. mcculls flagged that its
  regex only matched 'class', missing enum/interface/@interface, and
  would produce false MISMATCH lines for any such file (LogHandler.java,
  ParameterCollector.java, etc.).

references/advice-class.md
- Rewrite the 'onExit resilient to onEnter throwing' section — the
  claim that 'onThrowable = Throwable.class ensures exit fires even on
  onEnter exception' was factually wrong. Per
  docs/how_instrumentations_work.md:532-552, 'if the OnMethodEnter
  method throws an exception, the OnMethodExit method is not invoked' —
  unconditionally; onThrowable cannot override it. onThrowable controls
  exit-on-target-method-throw, not exit-on-enter-throw. (mcculls)
- Add inline note that java.nio.charset.StandardCharsets is a java.nio.*
  type and forbidden in bootstrap instrumentations (per the same file's
  Must NOT list). In bootstrap advice, use the string charset name
  ('UTF-8') instead. (Copilot)

references/context-tracking.md
- Soften the 'rxjava-2.0 hooks subscribe(Observer)' statement. The
  module's actual matcher is named('subscribe').and(takesArguments(1)),
  matching any single-arg subscribe overload with the argument typed
  as the base callback interface. Direct the reader at the module
  source instead of copying overload names. (Copilot)

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

fix(skill): trim bootstrap note on charset to a single line

Copilot's suggestion was 'add an explicit note here'; the initial fix
was a full paragraph. Trimming to a single-sentence pointer since the
Must NOT list already carries the details.

Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com>

review feedback

Fix docs inconsistencies

Rename skill to apm-integrations

remove redundant note

Renamed .claude/skills to .agents/skills and added symlink-style redirects for the old location

Align NIO docs advice with skill

Simplify SKILL

Fix constructor advice best-practice

Address codex comments

Co-authored-by: mcculls <stuart.mcculloch@datadoghq.com>
Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: others All other instrumentations tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: feature Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants