Commit 084b01b
skill(apm-integrations): additional rules from recent HTTP-category PR 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>1 parent 48ac26b commit 084b01b
6 files changed
Lines changed: 236 additions & 6 deletions
File tree
- .agents/skills/apm-integrations/references
Lines changed: 45 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
92 | 92 | | |
93 | 93 | | |
94 | 94 | | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
95 | 109 | | |
96 | 110 | | |
97 | 111 | | |
| |||
107 | 121 | | |
108 | 122 | | |
109 | 123 | | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
Lines changed: 60 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
60 | 60 | | |
61 | 61 | | |
62 | 62 | | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
Lines changed: 54 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
39 | 39 | | |
40 | 40 | | |
41 | 41 | | |
42 | | - | |
| 42 | + | |
43 | 43 | | |
44 | | - | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
45 | 70 | | |
46 | 71 | | |
47 | | - | |
48 | | - | |
49 | | - | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
50 | 75 | | |
51 | 76 | | |
52 | 77 | | |
53 | | - | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
54 | 81 | | |
55 | 82 | | |
56 | 83 | | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
57 | 90 | | |
58 | 91 | | |
59 | 92 | | |
| |||
88 | 121 | | |
89 | 122 | | |
90 | 123 | | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
56 | 56 | | |
57 | 57 | | |
58 | 58 | | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
59 | 96 | | |
60 | 97 | | |
61 | 98 | | |
| |||
0 commit comments