skill(apm-integrations): add rules from recent reviews + refactor into sub references by jordan-wong · Pull Request #11760 · DataDog/dd-trace-java

jordan-wong · 2026-06-29T04:56:30Z

Summary

Adds ~20 rules to the canonical apm-integrations skill and refactors it from a single 794-line SKILL.md into a 215-line routing overview + 6 topic-oriented reference files under references/. Every rule either encodes a pattern already in dd-java-agent/instrumentation/ or is enforced by CI.

Structure after refactor

.agents/skills/apm-integrations/
├── SKILL.md 215 lines — Steps 1-12 routing overview
└── references/
├── advice-class.md 167 lines — Step 7 (highest-risk step)
├── context-tracking.md 102 lines — Step 4.1 (new: span-creating vs context-tracking)
├── instrumenter-module.md 104 lines — Step 5
├── muzzle.md 125 lines — Step 9.2
├── naming-conventions.md 27 lines — Step 4 + 4.2
└── tests.md 142 lines — Step 9.1

SKILL.md's Steps 1-12 headings and the bodies of Steps 1-3, 6, 8, and 10-12 are unchanged from master. Content growth is in the reference files.

Where the rules came from

Reviewer feedback on recent toolkit generated library-instrumentation PRs — @PerfectSlayer and @amarziali left substantive comments on [DO NOT MERGE] feat(sparkjava): Add sparkjava-gen-2.3 module (toolkit-generated) #11337, [DO NOT MERGE] Apm ai toolkit new integration rxjava3 #11527, feat(sparkjava-2.3): toolkit-generated sparkjava-2.3 instrumentation [DO NOT MERGE] #11562, feat(feign-10.8): toolkit-generated feign HTTP client instrumentation #11709, and feat(commons-httpclient-2.0): toolkit-generated regeneration #11717. Each comment was distilled into a durable rule so the same feedback wouldn't recur.
CI enforcement — rules that formalize what spotbugs, muzzle, checkInstrumenterModuleConfigurations, Enforce Groovy Migration, and config-inversion-linter already block.

Suggested review order

SKILL.md — validate the routing overview.
references/context-tracking.md — the only entirely new territory. Formalizes the pattern from dd-java-agent/instrumentation/rxjava/rxjava-2.0/ (extends InstrumenterModule.ContextTracking).
references/advice-class.md + instrumenter-module.md — largest bodies of rules; highest reviewer value.
references/muzzle.md, tests.md, naming-conventions.md — mostly formalize existing conventions; skim.

Skip the refactor commits (structural moves, no content changes). Diff against master for the final content.

Rule provenance

Rule	Source
`context-tracking.md` — write `InstrumenterModule.ContextTracking` for reactive/async libs	In-repo pattern: `rxjava-2.0`
`context-tracking.md` — Flowable overload rule (hook framework-internal overload)	@amarziali on #11527
`context-tracking.md` — parent-child bridging test pattern	In-repo pattern: `rxjava-2.0/src/test/`
`naming-conventions.md` — dir name must end with version or `-common`/`-stubs`/`-iast`	CI: `buildSrc/.../InstrumentationNamingPlugin.kt`
`naming-conventions.md` — filename ↔ class name must match	Java language rule (`javac` enforces at compile)
`instrumenter-module.md` — interface-only API JARs need `ForTypeHierarchy` + `implementsInterface`	In-repo pattern: `jms/javax-jms-1.1`
`instrumenter-module.md` — no static constants for one-shot methods	In-repo convention
`instrumenter-module.md` — no single-type helper for `CallDepthThreadLocalMap`	@amarziali on #11527
`instrumenter-module.md` — `instrumentationNames()` version-alias	In-repo convention
`instrumenter-module.md` — preserve master's integration name when regenerating	@PerfectSlayer on #11717
`advice-class.md` — single delegate method, not all overloads	@amarziali on #11527
`advice-class.md` — `onThrowable` semantics (exit-on-target-throw)	`docs/how_instrumentations_work.md:532-552`
`advice-class.md` — explicit charset when converting `byte[]` → `String`	SpotBugs `DM_DEFAULT_ENCODING`
`advice-class.md` — no `NullPointerException` catches	SpotBugs `DCN_NULLPOINTER_EXCEPTION`
`advice-class.md` — `@AppliesOn` for multiple advices	`docs/how_instrumentations_work.md`
`tests.md` — Java tests only, no new `.groovy` files	CI: `Enforce Groovy Migration` workflow
`tests.md` — register names in `metadata/supported-configurations.json`	CI: `checkInstrumenterModuleConfigurations` + `config-inversion-linter`
`tests.md` — cover error/exception scenarios	@PerfectSlayer reviews
`tests.md` — `compileOnly` vs `testImplementation` version split	@amarziali on #11562
`tests.md` — prior-version module in `testImplementation`	In-repo pattern: jedis-3.0
`muzzle.md` — `assertInverse=true` traps + Pattern A/B	@amarziali on #11527
`muzzle.md` — exclude incompatible major versions	@PerfectSlayer on #11717
`muzzle.md` — `skipVersions` for malformed release versions	Observed failure on jedis `jedis-3.6.2`

Review feedback addressed

Round 1 of reviews (2026-07-08, from @mcculls and Copilot) surfaced 11 substantive issues. All addressed in 13d6bd32ab and d4600a4624:

SKILL.md Step 4 source layout no longer contradicts Step 9.1 (src/test/java/ throughout)
naming-conventions.md dropped the invented gRPCInstrumentation example and the buggy sanity-check script
tests.md error-test example now uses AgentSpan + span.getTag() (was OpenTelemetry's SpanData)
tests.mdsupported-configurations.json template default is now "true" (was "false")
tests.md no-groovy enforcement now references the real workflow (Enforce Groovy Migration), not a made-up bot name
context-tracking.md rxjava-2.0 subscribe reference softened to describe the actual matcher pattern
advice-class.mdonThrowable semantics rewritten per docs/how_instrumentations_work.md:532-552 (the earlier "SAFER" claim was factually wrong)

Paired with

DataDog/apm-instrumentation-toolkit#472 — the toolkit's derived copy of this skill. Under the current sync policy, changes flow dd-trace-java → toolkit; the toolkit-side PR is downstream and will be updated after this one merges.

dd-octo-sts · 2026-06-29T05:23:56Z

🟢 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.92 s	13.98 s	[-1.0%; +0.2%] (no difference)
startup:insecure-bank:tracing:Agent	12.93 s	13.04 s	[-1.8%; -0.0%] (maybe better)
startup:petclinic:appsec:Agent	16.93 s	16.23 s	[-0.2%; +8.9%] (no difference)
startup:petclinic:iast:Agent	16.42 s	16.98 s	[-7.7%; +1.1%] (no difference)
startup:petclinic:profiling:Agent	16.53 s	16.86 s	[-3.1%; -0.9%] (maybe better)
startup:petclinic:sca:Agent	16.91 s	16.77 s	[-0.3%; +2.0%] (no difference)
startup:petclinic:tracing:Agent	16.11 s	15.71 s	[-1.7%; +6.8%] (no difference)

Commit:8394775c · 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.

Copilot

Pull request overview

Ports and restructures the add-apm-integrations Claude skill documentation to incorporate recently reviewer-encoded rules (including a new context-tracking/category-B axis) while splitting the previously-large SKILL.md into a shorter routing overview plus topic-focused reference docs.

Changes:

Refactors SKILL.md into a concise step routing guide that links out to detailed reference files.
Adds new reference documents covering context-tracking vs span-creating guidance, naming, InstrumenterModule rules, advice rules, tests, and muzzle directives.
Updates SKILL step content to point to the new references and codify additional “gotcha” rules (e.g., latestDep version range alignment).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
.claude/skills/add-apm-integrations/SKILL.md	Refactored into a shorter step overview with links to new reference docs; adds new Step 4.1/4.2 routing guidance.
.claude/skills/add-apm-integrations/references/context-tracking.md	New detailed guidance for context-tracking (async/reactive) instrumentations.
.claude/skills/add-apm-integrations/references/naming-conventions.md	New naming rules for module directories and Java class/file naming consistency.
.claude/skills/add-apm-integrations/references/instrumenter-module.md	New detailed InstrumenterModule rules (interfaces, helper declarations, naming preservation, etc.).
.claude/skills/add-apm-integrations/references/advice-class.md	New detailed advice-class rules and “must/must-not” guidance.
.claude/skills/add-apm-integrations/references/tests.md	New testing guidance focusing on Java/JUnit5 and required scenarios + config registration.
.claude/skills/add-apm-integrations/references/muzzle.md	New muzzle directive patterns and common failure modes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mcculls

Claude found four issues in the new reference files — details in the inline comments below.

jordan-wong · 2026-07-08T13:01:06Z

For some context, how I create these prompt updates goes like:
-> have doc in toolkit research branch to track items/ rules to add to skills
-> at each step of evaluation process for generated PR process:

collect feedback
make changes if fix needed
add rules to running list if fix seems generalizeable

Evaluation process:
Generate PRs with toolkit -> CI test failures -> LLM review -> human review feedback

@mcculls

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>

datadog-official · 2026-07-08T14:49:00Z

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 56.92% (-0.02%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 8394775 | Docs | Datadog PR Page | Give us feedback!}

dd-octo-sts · 2026-07-08T15:15:39Z

Hi! 👋 Thanks for your pull request! 🎉

To help us review it, please make sure to:

Add at least one type, and one component or instrumentation label to the pull request

If you need help, please check our contributing guidelines.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:d4600a4624

ℹ️ 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".

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

PerfectSlayer

Left minor comment about testing

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

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>

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>

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>

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>

@mcculls

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>

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>

…rects for the old location

dd-octo-sts · 2026-07-09T19:02:19Z

/merge

gh-worker-devflow-routing-ef8351 · 2026-07-09T19:02:23Z

View all feedbacks in Devflow UI.

2026-07-09 19:02:23 UTC ℹ️ Start processing command /merge

2026-07-09 19:02:28 UTC ℹ️ MergeQueue: pull request added to the queue

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

2026-07-09 19:12:40 UTC 🚨 MergeQueue: This merge request is in error because of DDCI

DDCI didn't respond in time.
If this continues, please let us know in #ci-infra-support with the following details:

Head SHA of the MQ branch: 085e35b
Name of the MQ branch: mq-working-branch-master-1b8e61a
The DDCI Workflow from GenerateDDCIRequestFromDevflow: atlas

dd-octo-sts · 2026-07-09T20:55:16Z

/merge

gh-worker-devflow-routing-ef8351 · 2026-07-09T20:55:21Z

View all feedbacks in Devflow UI.

2026-07-09 20:55:21 UTC ℹ️ Start processing command /merge

2026-07-09 20:55:25 UTC ℹ️ MergeQueue: pull request added to the queue

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

2026-07-09 21:53:12 UTC ℹ️ MergeQueue: This merge request was merged

@ValentinZakharov

…R reviews (#11927) skill(apm-integrations): HTTP client follow-up rules from PR reviews Five rules distilled from HTTP-client and HTTP-server PR reviews (feign #11709, commons-httpclient #11717, sparkjava #11708) that were NOT covered by the R13-R33 additions in #11760. - advice-class.md: async double-span (do not wrap an async client with advice when the sync delegate is already instrumented; produces two spans per request). Source: @ValentinZakharov on PR #11709. - advice-class.md: HelperMethods refactor anti-pattern (do not extract advice logic into a per-instrumentation helper class just to shorten the advice body — inline unless genuinely shared). Source: @ygree on PR #11717. - instrumenter-module.md: regeneration must preserve every override the master version has — not just super(...), but also defaultEnabled(), helperClassNames(), contextStore(), orderPriority(), etc. Source: Codex reviews on PRs #11709 and #11708 (recurring pattern). - instrumenter-module.md: scan dd-java-agent/instrumentation/$framework/ before generating; do not create parallel duplicate modules. Source: @PerfectSlayer's historical review on #10941, still an unencoded gap. - muzzle.md: base testImplementation dep version must match the module's declared minimum, not the latest — extends the existing latestDep parity rule to base tests. Source: @PerfectSlayer on PR #11708. 3 files, +44 lines. No changes to existing rules. Reviewers: @mcculls (skill hygiene, reviewed #11760), @PerfectSlayer (HTTP domain, contributed rules from #10941 + #11708). skill(apm-integrations): additional rules from HTTP feedback audit Adds 9 rules across 5 files based on extended audit of PR #11708 (sparkjava), PR #11709 (feign), and the sparkjava CI-fix commit history: - context-tracking: CompletableFuture cancellation preservation (codex, #11709) - advice-class: framework-inside-framework span identity (codex, #11708) - advice-class: no non-constant static fields in advice (CI-fix commit f6d1263) - tests: 5 test-hygiene rules (PerfectSlayer, #11708) — no Thread.sleep, static server field, shared test bases, ForkedTest justification, no default jvmArgs - supported-configurations: registry type correctness (CI-fix commits 53836a0, 48a84c0) - instrumenter-module: helperClassNames() for enrichment helpers (CI-fix commit 2c372a3) skill(apm-integrations): condition super(...) naming on sibling structure Reconciles two conflicting maintainer positions on the constructor pattern: - Stuart McCulloch (PR #11760 comment 3552616459) — new instrumentations should adopt the version-alias pattern - Valentin Zakharov (PR #11709 comment 3532152120) — single-module frameworks should pass one name; extra names mint DD_TRACE_<NAME>_ENABLED flags with no counterpart to gate against The empirical convention in dd-trace-java confirms Valentin's rule for single-module frameworks (feign, freemarker, liberty, sparkjava all pass one name; freemarker and liberty do so even with real version siblings) and McCulloch's rule for frameworks with real sibling versions (okhttp shipping okhttp-2.0 and okhttp-3.0 with a shared 'okhttp' group flag). Also fixes the previous jedis example which claimed 'jedis-3.0' alias but the shipping jedis-3.0 module actually passes super("jedis", "redis"). skill(apm-integrations): address Copilot review comments on #11927 7 findings, all verified against master: - tests.md: waitForTraces(N) waits for >=N (not exactly N) with 20s bounded timeout per ListWriter.java - supported-configurations.md: fix internal contradiction — registry uses "decimal" (rates) and "int" (counts), NOT "double"/"integer" (matches canonical guidance earlier in same doc, line 52) - instrumenter-module.md: replace non-existent super("feign") example with freemarker (real, verifiable); replace misleading single-name list with an accurate one; add sparkjava-2.3 counter-example explaining why super("sparkjava","sparkjava-2.4") uses the -2.4 alias despite living in the -2.3/ directory (compile against 2.3, test against 2.4 for JettyHandler) - muzzle.md: reframe testImplementation=min as default preference, not absolute rule; document justified deviation with sparkjava-2.3 build.gradle as the canonical example - context-tracking.md: reframe CompletableFuture pattern to emphasize that the correct (non-reassigning) pattern does NOT require @Advice.Return(readOnly=false); only add readOnly=false if you have a documented reason to substitute the return value - advice-class.md: S1 framework-in-framework example now includes the null-check on activeSpan(), matching sparkjava's RoutesInstrumentation.java:52-55 (withRoute() does not guard) Copilot's citations were checked against master before applying. skill(apm-integrations): scrub audit-trail phrasing from shipping content - Reword "regenerating an existing module" as "rewriting or refactoring" — the guidance applies whenever an existing module is being changed, not just to automated regeneration flows. - Remove the "Rationale traceable to reviewer comments (PR/comment IDs)" footnote. That attribution belongs in commit messages and PR descriptions, not in the shipping skill; readers of the skill just need the rule. skill(apm-integrations): address second-round Codex review comments Four findings, all P2, all verified against master: - advice-class.md (framework-in-framework): narrow the rule to route-only enrichers (SparkJava). JAX-RS annotations and Ratpack legitimately create handler/controller spans in addition to the outer server span (jax-rs-annotations-2.0/JaxRsAnnotationsInstrumentation.java:128; ratpack-1.5/TracingHandler.java:41). Original blanket wording would cause future JAX-RS/Ratpack instrumentation to drop expected child spans. - advice-class.md (async wrapper): completion-only propagation is not enough when the sync delegate runs on a worker thread — the sync client's advice creates its span BEFORE the future completes. Document the two supported approaches: (1) rely on executor instrumentation, or (2) reactivate around the delegate submission via a shared wrapper. Reference java-concurrent-1.8's existing patterns. - context-tracking.md (CompletableFuture): the CORRECT example was itself wrong on two counts caught by Codex: (a) missing onThrowable = Throwable.class means the exit advice skips when the instrumented method throws before returning the future — any span started on enter would leak (b) used a lambda body, which the "no lambdas in advice methods" rule in advice-class.md:111 explicitly forbids — lambdas compile to synthetic classes that are not helper-injected Rewrite the CORRECT example with @Advice.Thrown handling, a named BiConsumer helper (ClientCompletionCallback) instead of a lambda, and helperClassNames() implications. skill(apm-integrations): address mcculls review on instrumenter-module.md Merge branch 'master' into feat/skill-http-followup-rules Merge branch 'master' into feat/skill-http-followup-rules Co-authored-by: sarah.chen <sarah.chen@datadoghq.com>

jordan-wong self-assigned this Jul 8, 2026

mcculls requested review from Copilot and mcculls July 8, 2026 10:44

Copilotstarted reviewing on behalf of mcculls July 8, 2026 10:45 View session

CopilotAI reviewed Jul 8, 2026
View reviewed changes

mcculls reviewed Jul 8, 2026
View reviewed changes

jordan-wong requested a review from Copilot July 8, 2026 15:12

Copilotstarted reviewing on behalf of jordan-wong July 8, 2026 15:12 View session

jordan-wong changed the title ~~skill(add-apm-integrations): R13-R33 + Cat B (context-propagation) rules~~skill(add-apm-integrations): add rules from recent reviews + progressive-disclosure refactorJul 8, 2026

jordan-wong changed the title ~~skill(add-apm-integrations): add rules from recent reviews + progressive-disclosure refactor~~skill(add-apm-integrations): add rules from recent reviews + refactor into sub referencesJul 8, 2026

jordan-wong marked this pull request as ready for review July 8, 2026 15:15

jordan-wong requested a review from a team as a code owner July 8, 2026 15:15

jordan-wong requested a review from ygree July 8, 2026 15:15

jordan-wong requested review from mcculls and removed request for ygree July 8, 2026 15:16

CopilotAI reviewed Jul 8, 2026
View reviewed changes

chatgpt-codex-connectorBot reviewed Jul 8, 2026
View reviewed changes

mcculls requested a review from Copilot July 9, 2026 11:41

Copilotstarted reviewing on behalf of mcculls July 9, 2026 11:45 View session

CopilotAI reviewed Jul 9, 2026
View reviewed changes

PerfectSlayer reviewed Jul 9, 2026
View reviewed changes

Comment thread.claude/skills/add-apm-integrations/SKILL.md Outdated

mcculls requested a review from Copilot July 9, 2026 12:27

CopilotAI reviewed Jul 9, 2026
View reviewed changes

mcculls requested a review from Copilot July 9, 2026 13:38

Copilotstarted reviewing on behalf of mcculls July 9, 2026 13:39 View session

jordan-wongand others added 15 commits July 9, 2026 18:48

review feedback
c8cdffb

Fix docs inconsistencies
be9be93

Rename skill to apm-integrations
2b44002

remove redundant note
594637c

Renamed .claude/skills to .agents/skills and added symlink-style redi…
49b1b8d
…rects for the old location

Align NIO docs advice with skill
45952d1

Simplify SKILL
30fc5d3

Fix constructor advice best-practice
04208a8

Address codex comments
8394775

mcculls force-pushed the skill/r13-r33-from-toolkit branch from b5d44e0 to 8394775Compare July 9, 2026 17:50

jordan-wong enabled auto-merge July 9, 2026 18:25

jordan-wong added this pull request to the merge queue Jul 9, 2026

github-merge-queueBot removed this pull request from the merge queue due to failed status checks Jul 9, 2026

mcculls added this pull request to the merge queue Jul 9, 2026

github-merge-queueBot removed this pull request from the merge queue due to failed status checks Jul 9, 2026

gh-worker-dd-mergequeue-cf854dBot merged commit fc6f5e4 into masterJul 9, 2026
588 checks passed

gh-worker-dd-mergequeue-cf854dBot deleted the skill/r13-r33-from-toolkit branch July 9, 2026 21:53

github-actionsBot added this to the 1.65.0 milestone Jul 9, 2026

jordan-wong mentioned this pull request Jul 13, 2026
skill(apm-integrations): additional rules from recent HTTP-category PR reviews #11927
Merged

Uh oh!

Conversation

jordan-wong commented Jun 29, 2026• edited by mcculls Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Structure after refactor

Where the rules came from

Suggested review order

Rule provenance

Review feedback addressed

Paired with

Uh oh!

dd-octo-stsBot commented Jun 29, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 Java Benchmark SLOs — All performance SLOs passed

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcculls left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jordan-wong commented Jul 8, 2026

Uh oh!

datadog-officialBot commented Jul 8, 2026• edited by datadog-prod-us1-4 BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dd-octo-stsBot commented Jul 8, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connectorBot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PerfectSlayer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

jordan-wong commented Jun 29, 2026•
edited by mcculls
Loading

dd-octo-stsBot commented Jun 29, 2026•
edited
Loading

mcculls left a comment •
edited
Loading

datadog-officialBot commented Jul 8, 2026•
edited by datadog-prod-us1-4 Bot
Loading

gh-worker-devflow-routing-ef8351Bot commented Jul 9, 2026•
edited
Loading

gh-worker-devflow-routing-ef8351Bot commented Jul 9, 2026•
edited
Loading