skill(apm-integrations): additional rules from recent HTTP-category PR reviews#11927
Conversation
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).
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)
…ture 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").
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Behaviour check — how the current draft affects generated outputUpdate (2026-07-13 pm) — original findings retracted. After posting this comment I extracted the compile-step agent trace and discovered the blind regens did NOT actually load the draft skill content in this PR. The toolkit's skill-loader first-match preferred the target repo's in-tree Preserving the original observations below for the record, but with corrections: Two candidates:
Static-verification results — corrected classification:
Corrected reading: the rule content in this PR is unchanged and still worth reviewing on its merits. The empirical validation evidence in my original comment is retracted pending a re-run against a properly-loaded skill. I'm setting up that re-run now on an async-category cycle (rxjava-3.0) with the skill correctly loaded via a target-repo branch that has the draft in-tree; results will follow if useful. Sorry for the noise on the earlier reading. |
There was a problem hiding this comment.
Pull request overview
Extends the canonical apm-integrations skill reference docs with additional rules extracted from recent HTTP-category review/CI feedback, and refines guidance around integration naming via super(...) based on version-sibling structure.
Changes:
- Adds “test hygiene” rules for instrumentation tests (deterministic waits, embedded server lifecycle, base-class factoring, forked-test justification, avoiding default JVM args).
- Adds registry population guidance for
metadata/supported-configurations.json(required_ENABLEDentries, analytics key shapes, common mistakes). - Expands integration authoring guidance around module discovery,
super(...)naming choices, async wrapper double-span avoidance,CompletableFuturecancellation preservation, and advice-class constraints.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
.agents/skills/apm-integrations/references/tests.md |
Adds test-hygiene rules intended to reduce flakiness and CI cost. |
.agents/skills/apm-integrations/references/supported-configurations.md |
Adds common-mistake guidance for correctly registering new configuration entries. |
.agents/skills/apm-integrations/references/muzzle.md |
Adds a rule tying test dependency versions to declared module ranges/min versions. |
.agents/skills/apm-integrations/references/instrumenter-module.md |
Adds “scan first” guidance, reconciles super(...) naming rules, and adds helper declaration guidance. |
.agents/skills/apm-integrations/references/context-tracking.md |
Adds guidance to preserve cancellation semantics for async return types. |
.agents/skills/apm-integrations/references/advice-class.md |
Adds rules for async double-span avoidance, helper-method anti-pattern, framework-in-framework enrichment, and advice static-field constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
…tent - 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.
jordan-wong
changed the title
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Documentation-only PR extending APM integration guidance with rules from recent reviewer feedback. All code examples verified against actual module implementations (sparkjava-2.3, jedis, okhttp, freemarker, liberty); naming reconciliation is empirically grounded and consistent; cross-references validated. No executable code changes — no regressions possible.
🤖 Datadog Autotest · Commit 04a0719 · What is Autotest? · Any feedback? Reach out in #autotest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04a07191ee
ℹ️ 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".
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.
Automated integration-authoring pass on rxjava-3.0, produced against the draft skill in #11927 (which was in-tree on the target checkout at generation time — using the skill-loading precedence fix from the 2026-07-13 HTTP cycle correction). Reference for #11927 reviewers to see concrete output the current skill draft produces. Not intended to be merged as-is. Base: master @ 05671ce (rebased from original bd134f3; the original branch inherited a stash-merge revert of #11852 during eval branch setup that dropped DD_TRACE_ROBOLECTRIC_ENABLED from metadata plus 172 unrelated files. This rebased version cherry-picks ONLY the rxjava-3.0 module output onto current master, preserving all unrelated upstream changes.) Cost: \$43.23, wall time ~3.5h, reviewer approved with 0 todos remaining. Diff scope: dd-java-agent/instrumentation/rxjava/rxjava-3.0/ only. Metadata: no change (all rxjava3 entries already present on master).
…skill Automated integration-authoring pass on reactor-core-3.1, produced against the draft skill in #11927. Reference for #11927 reviewers to see concrete output the current skill draft produces. Not intended to be merged as-is. Base: master @ 05671ce (rebased from original 2e1d2b9; the original branch inherited a stash-merge revert of #11852 during eval branch setup that dropped DD_TRACE_ROBOLECTRIC_ENABLED from metadata plus 172 unrelated files. This rebased version cherry-picks ONLY the reactor-core-3.1 module output onto current master, preserving all unrelated upstream changes.) Cost: \$13, reviewer approved with 0 todos remaining. Diff scope: dd-java-agent/instrumentation/reactor-core-3.1/ only. Metadata: no change (all reactor entries already present on master). Note: the toolkit reorganized the package layout from 'datadog.trace.instrumentation.reactor.core' (master) to 'datadog.trace.instrumentation.reactorcore' (eval). This is a regen-preservation Rule #2 concern; documented for reviewer consideration.
| The version alias (e.g. `"jedis-3.0"`) maps to `DD_TRACE_JEDIS_3_0_ENABLED`. Do NOT add version aliases to the decorator's `instrumentationNames()` — that method is for analytics keys only. | ||
| Users can then set `DD_TRACE_OKHTTP_ENABLED=false` (group off) OR `DD_TRACE_OKHTTP_3_ENABLED=false` (this version only). | ||
|
|
||
| **New module you expect will imminently sibling** — add the alias upfront and document why in the commit message. If no sibling appears, drop the alias in a follow-up. |
There was a problem hiding this comment.
| **New module you expect will imminently sibling** — add the alias upfront and document why in the commit message. If no sibling appears, drop the alias in a follow-up. | |
| **New module you expect will soon have a sibling** — add the alias upfront and document why in the commit message. If no sibling appears, drop the alias in a follow-up. |
There was a problem hiding this comment.
Applied your suggestion verbatim in 3c2214d6f2 — "soon have a sibling."
|
|
||
| **Existing module** (modifying, refactoring, or splitting): read the existing module's `super(...)` and copy it verbatim. Integration names are public config API — renaming one silently breaks customer `DD_TRACE_*_ENABLED` settings. | ||
|
|
||
| Before choosing, run `ls dd-java-agent/instrumentation/$framework/` — the directory contents are the ground truth for whether siblings exist. |
There was a problem hiding this comment.
| Before choosing, run `ls dd-java-agent/instrumentation/$framework/` — the directory contents are the ground truth for whether siblings exist. | |
| Before choosing, list the `dd-java-agent/instrumentation/$framework/` directory — the contents are the ground truth for whether siblings exist. |
There was a problem hiding this comment.
Applied your suggestion verbatim in 3c2214d6f2 — "list the dd-java-agent/instrumentation/$framework/ directory — the contents are the ground truth for whether siblings exist."
|
|
||
| ## Enrichment helpers must be declared in `helperClassNames()` | ||
|
|
||
| If your advice delegates to a helper class (e.g. `SparkJavaRouteEnricher.enrich(...)` from inside `RoutesAdvice`), the helper's fully-qualified class name MUST be listed in `helperClassNames()` on the `InstrumenterModule`: |
There was a problem hiding this comment.
caveat: unless the helper is supplied on the boot-class-path, such as by agent-bootstrap
There was a problem hiding this comment.
Added in 3c2214d6f2 — the requirement now reads "MUST be listed in helperClassNames() on the InstrumenterModule — unless the helper is supplied on the boot-class-path (e.g. from agent-bootstrap), in which case it is already available without injection."
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 3b64d3e: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
DDCI pipeline didn't start (sourcing_failed)... Please retry. DDCI Change Request: |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 9e3b9f0:
What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 8605c11:
What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
PerfectSlayer
added
the
tag: no release notes
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
feat/skill-http-followup-rules
branch
Summary
Extends the
add-apm-integrationsskill with additional rules derived from reviewer feedback and CI-fix commits across recent HTTP-category PRs (#11708 sparkjava-2.3, #11709 feign-10.8, #11717 commons-httpclient-2.0). Also reconciles a conflict between two maintainer positions onsuper(...)naming.Follow-on to #11760.
What's added
Rules from reviewer comments (in the referenced PRs):
advice-class.md)advice-class.md)advice-class.md)HTTP_RESOURCE_DECORATOR.withRoute(...)— do NOT create a new span,Decorator, orAgentScope. Null-checkactiveSpan()first.CompletableFuture(context-tracking.md)future = future.whenComplete(...)—whenCompleteproduces a dependent stage, and cancelling that stage does not cancel the original requesttests.md)Thread.sleep(); embedded servers asstaticfields, not per-test; shared scaffolding in a base class;ForkedTestrequires a justified isolation reason; no defaultjvmArgsin test tasksinstrumenter-module.md)super(...),defaultEnabled(),helperClassNames(),contextStore(),orderPriority(),muzzleDirective()) — silent loss ofdefaultEnabled()=falseon an opt-in integration ships it on by defaultinstrumenter-module.md)dd-java-agent/instrumentation/$framework/$framework-$version/, list the parent directory to check for existing coverage; do not create parallel duplicate modulestestImplementationversion (muzzle.md)build.gradleheader comment)Rules from CI-fix commit history on #11708:
advice-class.md*Advice.java(muzzle contract — advice methods are inlined and non-constant statics get pulled into every callsite)supported-configurations.mdboolean,int(NOTinteger),decimal(NOTdouble),string,array,map. Everysuper(...)name needs a matching_ENABLEDentry; every_ANALYTICS_SAMPLE_RATEneeds a matching_ANALYTICS_ENABLED.instrumenter-module.mdhelperClassNames()— otherwise the target classloader lacks it and the advice throwsNoClassDefFoundErrorat runtimesuper(...)naming reconciliation (instrumenter-module.md)Two maintainer positions were in tension:
super("jedis", "jedis-3.0")).DD_TRACE_<NAME>_ENABLEDflag with no counterpart to gate against.Empirical check of the codebase: single-name is the norm even for versioned modules —
freemarker-2.3.9+freemarker-2.3.24bothsuper("freemarker");liberty-20.0+liberty-23.0bothsuper("liberty"). The multi-name form appears when frameworks have real cross-version group switches:okhttp(super("okhttp", "okhttp-3")becauseokhttp-2.0/sibling exists).Also worth noting: the prior skill example for the version-alias pattern used
super("jedis", "jedis-3.0"), but the shippingjedis-3.0module actually usessuper("jedis", "redis"), andDD_TRACE_JEDIS_3_0_ENABLEDdoes not exist inmetadata/supported-configurations.json.Resolution: encode as conditional guidance — pass ONE name unless multiple version-specific sibling directories genuinely exist.
sparkjava-2.3'ssuper("sparkjava", "sparkjava-2.4")is included as a nuanced counter-example (the-2.4alias reflects the version exercised by tests, not the directory name — see thebuild.gradleheader comment on that module).Both maintainer positions are correct in their respective situations; the reconciled guidance in this PR distinguishes them explicitly.
Files changed
.agents/skills/apm-integrations/references/advice-class.md.agents/skills/apm-integrations/references/context-tracking.md.agents/skills/apm-integrations/references/instrumenter-module.md.agents/skills/apm-integrations/references/muzzle.md.agents/skills/apm-integrations/references/supported-configurations.md.agents/skills/apm-integrations/references/tests.mdNot in scope
.agents/skills/apm-integrations/references/.