skill(apm-integrations): additional rules from recent HTTP-category PR reviews#11927
skill(apm-integrations): additional rules from recent HTTP-category PR reviews#11927jordan-wong wants to merge 5 commits into
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").
|
🎯 Code Coverage (details) 🔗 Commit SHA: 04a0719 | Docs | Datadog PR Page | Give us feedback! |
🟢 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".
| @Advice.OnMethodExit(suppress = Throwable.class) | ||
| public static void exit(@Advice.Return CompletableFuture<Response> future, | ||
| @Advice.Enter AgentSpan span) { | ||
| future.whenComplete((result, error) -> finishSpan(span, result, error)); |
There was a problem hiding this comment.
This “CORRECT” advice example still uses a lambda inside an @Advice.OnMethodExit, but the same skill’s advice guidance forbids lambdas in advice methods because they compile to synthetic call targets that are not helper-injected. When this pattern is copied into async client advice, ByteBuddy can inline a call site that references the advice class’s synthetic lambda method at runtime; existing callback advice uses declared callback/helper classes instead. Please show a named BiConsumer/helper callback while keeping the return read-only.
Useful? React with 👍 / 👎.
| } | ||
| ``` | ||
|
|
||
| No `AgentScope`, no `startSpan()`, no `decorator.afterStart()`. This applies to any inner routing/dispatch framework that runs inside another instrumented HTTP server. It does NOT apply to standalone HTTP clients, which own their own span identity. |
There was a problem hiding this comment.
This blanket rule says it applies to any inner routing/dispatch framework and lists Ratpack/JAX-RS, but current instrumentations for those contexts intentionally create framework/controller spans in addition to enriching the outer span: JaxRsAnnotationsAdvice calls startSpan in dd-java-agent/instrumentation/rs/jax-rs/.../JaxRsAnnotationsInstrumentation.java:128, and Ratpack does the same in dd-java-agent/instrumentation/ratpack-1.5/.../TracingHandler.java:41. If the skill is followed for those frameworks, new server framework instrumentation would drop expected child spans rather than merely avoiding Spark-style span rewriting; please narrow this to route-only enrichers or carve out frameworks that own a handler span.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // CORRECT — attach the callback for its side-effect; keep the return read-only | ||
| @Advice.OnMethodExit(suppress = Throwable.class) |
There was a problem hiding this comment.
The “CORRECT” async-client exit advice omits onThrowable = Throwable.class, so if a copied implementation starts a span on enter and the instrumented method throws before returning its future, ByteBuddy will skip this exit advice and the span/scope cleanup path will never run. This contradicts the advice lifecycle rule above in the same file; please include onThrowable = Throwable.class in the correct pattern and handle the thrown case separately from attaching the completion callback.
Useful? React with 👍 / 👎.
|
|
||
| If the target method delegates to a sync client that is already instrumented (common in async-wrapper classes like `AsyncFeignClient`, `AsyncHttpClient`, etc.), do NOT open a second span in the async wrapper. The sync client's advice already opens the client span; wrapping again produces two spans per request, with the outer span holding no additional context. | ||
|
|
||
| Before adding advice to an async wrapper, trace the call path to the sync delegate. If the delegate is already instrumented for span emission, the async wrapper only needs context-propagation advice (capture on submission, restore on completion) — not a second span. See `context-tracking.md` for the propagation pattern. |
There was a problem hiding this comment.
For async wrappers that submit the sync client call to a worker thread, restoring context only on completion is too late: the sync client's advice creates the HTTP span while the worker executes, before the future completes, so it will not see the captured parent and can be emitted as a root or under the wrong context. The wrapper still should not create a second client span, but the propagation advice needs to reactivate around the delegate/executor task (or rely on executor instrumentation) and separately around user completion callbacks.
Useful? React with 👍 / 👎.
|
For reviewer reference, the two blind regenerations described in the behaviour-check comment above are now up as their own draft PRs (not for merging):
Each PR body includes a per-rule adherence table against the rules being proposed in this PR, showing which ones the current output honours vs violates. The |
Async cycle reference regens — doneFollowing up on the earlier behaviour-check retraction: the async-category blind regens are done, this time with the skill-loading path correctly configured. Two new draft reference PRs:
Both were generated against the current draft of this PR's skill (F2, S1, CI-1, master-override preservation, etc.), and this time the trace confirms the draft skill was actually loaded (unlike the retracted 2026-07-13 attempt). The reactor run is notably smoother than rxjava3 (0 test-fix iterations, 1 reviewer iter, ~⅓ the cost) — could be Flux+Mono being simpler than rxjava's 6-type surface, or the skill guiding cleaner Cat B code. Reviewers may find that comparison useful signal. I'll be monitoring CI on both and fixing what I can. Any failures that require skill changes will feed back into this PR. |
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/.