skill(add-apm-integrations): add rules from recent reviews + refactor into sub references#11760
skill(add-apm-integrations): add rules from recent reviews + refactor into sub references#11760jordan-wong wants to merge 14 commits into
Conversation
…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>
🟢 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. |
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>
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>
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>
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>
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>
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>
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>
There was a problem hiding this comment.
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.mdinto 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.
| **Write Java tests (JUnit 5), NOT Groovy/Spock.** dd-trace-java policy | ||
| forbids new `.groovy` files — the PR bot (`checkNewGroovyFiles`) rejects any PR containing them. | ||
| All new tests must go in `src/test/java/`. |
There was a problem hiding this comment.
Fixed in 13d6bd32ab — replaced with the real workflow: Enforce Groovy Migration (.github/workflows/enforce-groovy-migration.yaml).
|
|
||
| **How to identify the right overload:** read the framework source. The public method usually calls a `subscribeActual(...)` or similar protected method that takes the framework-internal interface. If the framework documents one of the overloads as "for internal use only" or marks it `public final`, that's the implementation method — hook it. | ||
|
|
||
| **Reference:** dd-trace-java's `rxjava-2.0` hooks `subscribe(Observer)` (the implementation). |
There was a problem hiding this comment.
Fixed in 13d6bd32ab — softened to describe the actual matcher (named("subscribe").and(takesArguments(1))) rather than a specific overload.
|
|
||
| ## Step 4.1 – Span-creating vs context-tracking instrumentation | ||
|
|
||
| **Read [Context-Tracking Instrumentation](references/context-tracking.md) before picking instrumentation targets.** |
There was a problem hiding this comment.
Fixed — PR description now references context-tracking.md (file was renamed in b999e36c36).
| // CORRECT — explicit charset | ||
| import java.nio.charset.StandardCharsets; | ||
| String cmd = new String(commandBytes, StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Fixed in d4600a4624 — added a single-line note that StandardCharsets is java.nio.* and forbidden in bootstrap advice per the "Must NOT do" list below.
| client.execute(badRequest); | ||
| }); | ||
| List<List<SpanData>> traces = TEST_WRITER.waitForTraces(1); | ||
| SpanData span = traces.get(0).get(0); |
There was a problem hiding this comment.
SpanData is an OpenTelemetry SDK type (io.opentelemetry.sdk.trace.data.SpanData), not a dd-trace-java type. TEST_WRITER.waitForTraces() returns List<List<DDSpan>>, so traces.get(0).get(0) is a DDSpan. This example will not compile as written.
Correct pattern:
DDSpan span = writer.get(0).get(0);
assertNotNull(span.getTag("error.type"));
assertNotNull(span.getTag("error.message"));Error tags in dd-trace-java are plain string keys accessed via span.getTag(String) — there is no getAttributes() / AttributeKey API on DDSpan.
There was a problem hiding this comment.
Actually for tests AgentSpan should be enough
There was a problem hiding this comment.
Fixed in 13d6bd32ab — rewrote the example with AgentSpan + span.getTag() per your follow-up.
There was a problem hiding this comment.
Went with AgentSpan per this. Fixed in 13d6bd32ab.
| if (scope != null) scope.close(); | ||
| } | ||
|
|
||
| // SAFER — onThrowable = Throwable.class ensures exit fires even on onEnter exception |
There was a problem hiding this comment.
The comment here is factually wrong. onThrowable = Throwable.class on @Advice.OnMethodExit controls whether exit fires when the target method throws — not when the enter advice throws.
Per the project's own docs/how_instrumentations_work.md (lines 532–552):
- "If the
Advice.OnMethodEntermethod throws an exception, theAdvice.OnMethodExitmethod is not invoked." — this is unconditional;onThrowablecannot override it. - "
onThrowablecontrols whether exit fires when the instrumented method throws an exception."
When enter advice has suppress = Throwable.class, its exception is silently swallowed and the instrumented method continues running normally, so exit fires either way — but onThrowable had nothing to do with it.
Suggested replacement:
// SAFER — onThrowable = Throwable.class ensures exit fires even when the instrumented method throws
// suppress = Throwable.class on enter ensures enter-advice exceptions do not escape
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)There was a problem hiding this comment.
For own knowledge, don't necessarily add in to skill
- typically used for clearing scopes
- maybe not used if: recording some fact about method, if fails dont want to record that fact
There was a problem hiding this comment.
Fixed in 13d6bd32ab — rewrote section with correct semantics + citation to docs/how_instrumentations_work.md:532-552.
| { | ||
| "version": "A", | ||
| "type": "boolean", | ||
| "default": "false", |
There was a problem hiding this comment.
The "default": "false" here is wrong for the typical integration. Of the DD_TRACE_*_ENABLED keys in metadata/supported-configurations.json, ~83% of non-analytics enabled-flags default to "true" (jedis, okhttp, kafka, spring-web, etc.). "false" is reserved for integrations that override defaultEnabled() (OpenTelemetry, Hazelcast, sparkjava, etc.).
Hardcoding "false" here means a developer following the template registers every new integration as disabled by default. The checkInstrumenterModuleConfigurations linter only checks key presence — not the accuracy of the default value — so this passes CI silently.
Suggested fix: change the template to "default": "true" and add a note:
"default": "true" // use "false" only if the module overrides defaultEnabled()There was a problem hiding this comment.
Fixed in 13d6bd32ab — default is now "true"; added note that "false" is reserved for modules that override defaultEnabled().
| # Every public class declaration must match its filename | ||
| cd dd-java-agent/instrumentation/$framework/$framework-$version/src/main/java | ||
| for f in $(find . -name '*.java'); do | ||
| CLS=$(grep -oE 'public (final )?(abstract )?class [A-Za-z0-9_]+' "$f" | awk '{print $NF}') |
There was a problem hiding this comment.
The grep pattern public (final )?(abstract )?class [A-Za-z0-9_]+ only matches the class keyword, so it produces no output for public enum Foo, public interface Bar, or public @interface Baz. For any such file, CLS is empty and [ "" = "Foo" ] fires, printing a spurious MISMATCH.
These files are common in the instrumentation tree — e.g. LogHandler.java (public interface), ParameterCollector.java (public interface). Running this script would produce noise on every one of them.
A secondary issue: files with a nested public static class cause grep -oE to emit multiple lines, making CLS a newline-joined string that never equals EXPECTED.
Suggested fix:
for f in $(find . -name '*.java'); do
CLS=$(grep -oE '\bpublic\b[[:space:]]+(final[[:space:]]+|abstract[[:space:]]+)?(class|enum|interface|@interface)[[:space:]]+[A-Za-z0-9_]+' "$f" | head -1 | awk '{print $NF}')
EXPECTED=$(basename "$f" .java)
[ -z "$CLS" ] || [ "$CLS" = "$EXPECTED" ] || echo "MISMATCH: $f declares '$CLS'"
doneThere was a problem hiding this comment.
Fixed in 13d6bd32ab — dropped the sanity-check script entirely; javac already enforces filename ↔ public-class-name.
There was a problem hiding this comment.
we discussed just leaving a simple 1 line comment @mcculls but this actually seems fine to drop entirely IMO
|
For some context, how I create these prompt updates goes like:
Evaluation process: |
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>
|
🎯 Code Coverage (details) 🔗 Commit SHA: d4600a4 | Docs | Datadog PR Page | Give us feedback! |
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>
jordan-wong
changed the title
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. |
| **Read [Naming Conventions](references/naming-conventions.md) § "Java naming consistency".** | ||
|
|
||
| ### Must NOT do in InstrumenterModule | ||
| Filename and `public class` name MUST match character-for-character (including acronym casing). Use one canonical string everywhere: filename, class decl, `import static`, `ClassName.member` references. The reference file has a sanity-check script — run it before declaring done. |
| **Write Java tests (JUnit 5), NOT Groovy/Spock.** dd-trace-java policy | ||
| forbids new `.groovy` files — the `Enforce Groovy Migration` workflow | ||
| (`.github/workflows/enforce-groovy-migration.yaml`) rejects any PR containing them. | ||
| All new tests must go in `src/test/java/`. |
| - JUnit 5 test class in `src/test/java/datadog/trace/instrumentation/<framework>/` | ||
| - Verify: spans created, tags set, errors propagated, resource names correct | ||
| - Use `TEST_WRITER.waitForTraces(N)` for assertions | ||
| - Use `runUnderTrace("root", () -> { ... })` for synchronous code (Java lambda, not Groovy closure) |
| ```java | ||
| // Example error test (Java) | ||
| @Test | ||
| void testExceptionSetsErrorTags() throws Exception { | ||
| assertThrows(SomeException.class, () -> { | ||
| // trigger operation that throws | ||
| client.execute(badRequest); | ||
| }); | ||
| List<List<AgentSpan>> traces = TEST_WRITER.waitForTraces(1); | ||
| AgentSpan span = traces.get(0).get(0); | ||
| assertNotNull(span.getTag("error.type")); | ||
| assertNotNull(span.getTag("error.message")); | ||
| } | ||
| ``` |
| @ExtendWith(AgentJUnit5Extension.class) | ||
| public class Jedis3ClientTest extends AgentInstrumentationTest { |
| } | ||
| ``` | ||
|
|
||
| **Pattern B — Bounded range** (your instrumentation supports `[minVersion, maxVersion)` and an existing sibling module covers `[maxVersion, ∞)`). **Do NOT use `assertInverse = true`** — it can pick boundary versions inside your declared range as inverse-test targets, causing `muzzle-AssertFail-...` failures: |
| MUZZLE PASSED JedisInstrumentation BUT FAILURE WAS EXPECTED | ||
| ``` | ||
|
|
||
| Instead, declare BOTH the pass range AND the explicit fail ranges that bound it: |
| ``` | ||
| **Read [Muzzle Directives](references/muzzle.md).** | ||
|
|
||
| Two valid patterns: open-ended range (`[$min,)` with `assertInverse = true` only when the true min is verified) or bounded range (`[$min,$max)` with explicit `fail { versions = "[,$min)" }` — no `assertInverse`). Muzzle range must exclude incompatible major versions when the same `group:module` republishes with a rewritten API. Library-specific quirks (malformed release versions like `jedis-3.6.2`) require `skipVersions` — search adjacent modules for these before declaring new ranges. |
![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: 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".
| ### instrumentationNames() must include a version-qualified alias | ||
|
|
||
| ```java | ||
| @Override | ||
| public String[] instrumentationNames() { | ||
| // WRONG — only generic name | ||
| return new String[]{"jedis"}; | ||
|
|
||
| // CORRECT — generic + version alias | ||
| return new String[]{"jedis", "jedis-3.0"}; |
There was a problem hiding this comment.
For new version-specific enable/disable names, this needs to point agents at the InstrumenterModule constructor (super("jedis", "jedis-3.0")), not instrumentationNames(): InstrumenterModule exposes the configured integration names through its constructor/names list, while instrumentationNames() is a decorator method whose strings are checked as analytics keys by CheckDecoratorAnalyticsConfigTask. Following this example either adds a non-overriding method to the module or puts jedis-3.0 on the decorator, which will not make DD_TRACE_JEDIS_3_0_ENABLED control the module and will instead force unrelated analytics metadata.
Useful? React with 👍 / 👎.
| - Use `TEST_WRITER.waitForTraces(N)` for assertions | ||
| - Use `runUnderTrace("root", () -> { ... })` for synchronous code (Java lambda, not Groovy closure) |
There was a problem hiding this comment.
These Java-test bullets still reference the Groovy InstrumentationSpecification API: TEST_WRITER is not exposed by the Java JUnit harness, and Java tests in this repo extend datadog.trace.agent.test.AbstractInstrumentationTest, using assertTraces/writer plus helpers such as TraceUtils.runUnderTrace. If agents follow this text for new src/test/java instrumentation tests, the generated tests will not compile or will be written against the wrong harness.
Useful? React with 👍 / 👎.
|
|
||
| ### onExit handling when the target method throws | ||
|
|
||
| The `onThrowable = Throwable.class` attribute on `@Advice.OnMethodExit` controls whether the exit advice fires when the **instrumented target method** throws. Set it to `Throwable.class` (or omit it — this is the default) if you need to close the scope / finish the span regardless of whether the target method returned normally or threw. |
There was a problem hiding this comment.
When a span-finishing exit advice omits onThrowable, Byte Buddy does not call it if the instrumented method exits exceptionally (the OnMethodExit javadoc says the default skips exceptional termination: https://javadoc.io/doc/net.bytebuddy/byte-buddy/1.10.2/net/bytebuddy/asm/Advice.OnMethodExit.html). Following this sentence would leak the active scope/span on exceptions, so keep requiring onThrowable = Throwable.class for non-constructor span-finishing advice.
Useful? React with 👍 / 👎.
|
|
||
| ### Register new integration names in `metadata/supported-configurations.json` | ||
|
|
||
| Every new integration name in your module (whether from `super("foo-X.Y")` in `InstrumenterModule`, or from `instrumentationNames()` in the decorator) MUST have a corresponding entry in `metadata/supported-configurations.json` at the repo root. The `dd-gitlab/config-inversion-linter` CI job fails otherwise. |
There was a problem hiding this comment.
checkInstrumenterModuleConfigurations derives DD_TRACE_<NAME>_ENABLED only from InstrumenterModule super(...) args, while decorator instrumentationNames() are validated by checkDecoratorAnalyticsConfigurations as DD_TRACE_<NAME>_ANALYTICS_*. When a decorator has a name that is not also a module name, following this sentence adds the wrong _ENABLED key and still leaves CI failing for the missing analytics keys.
Useful? React with 👍 / 👎.
| // CORRECT — use CallDepthThreadLocalMap directly with the Advice or Decorator class as key | ||
| if (CallDepthThreadLocalMap.incrementCallDepth(GsonInstrumentation.class) > 0) return; | ||
| // ... in exit: | ||
| CallDepthThreadLocalMap.reset(GsonInstrumentation.class); |
There was a problem hiding this comment.
This “CORRECT” example uses GsonInstrumentation.class from inside advice, but instrumentation/module classes are not helper-injected into the application classloader, and this file also says advice must not reference the InstrumenterModule class. If an agent copies this pattern into inlined advice, the transformed application method can retain a class literal for a non-visible Datadog instrumentation class; use a target library class or declared helper/decorator class as the call-depth key instead.
Useful? React with 👍 / 👎.
| For complex frameworks with multiple version-specific or feature-specific instrumentations, you can group them under a single `InstrumenterModule` (file ending in `Module.java`). The module class: | ||
|
|
||
| - Must extend a `TargetSystem` subclass and have `@AutoService(InstrumenterModule.class)` | ||
| - Must implement `typeInstrumentations()` returning an array of instrumentations |
There was a problem hiding this comment.
InstrumenterModule.typeInstrumentations() returns List<Instrumenter> in this codebase, and existing grouped modules override that signature with a List, not an array. Following this bullet literally makes the grouped module fail to override the method (or fail compilation), so the guidance should say to return a List<Instrumenter>.
Useful? React with 👍 / 👎.
Summary
Adds ~20 rules to the canonical
add-apm-integrationsskill and refactors it from a single 794-line SKILL.md into a 215-line routing overview + 6 topic-oriented reference files underreferences/. Every rule either encodes a pattern already indd-java-agent/instrumentation/or is enforced by CI.Structure after refactor
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
spotbugs, muzzle,checkInstrumenterModuleConfigurations,Enforce Groovy Migration, andconfig-inversion-linteralready block.Suggested review order
SKILL.md— validate the routing overview.references/context-tracking.md— the only entirely new territory. Formalizes the pattern fromdd-java-agent/instrumentation/rxjava/rxjava-2.0/(extendsInstrumenterModule.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
masterfor the final content.Rule provenance
context-tracking.md— writeInstrumenterModule.ContextTrackingfor reactive/async libsrxjava-2.0context-tracking.md— Flowable overload rule (hook framework-internal overload)context-tracking.md— parent-child bridging test patternrxjava-2.0/src/test/naming-conventions.md— dir name must end with version or-common/-stubs/-iastbuildSrc/.../InstrumentationNamingPlugin.ktnaming-conventions.md— filename ↔ class name must matchjavacenforces at compile)instrumenter-module.md— interface-only API JARs needForTypeHierarchy+implementsInterfacejms/javax-jms-1.1instrumenter-module.md— no static constants for one-shot methodsinstrumenter-module.md— no single-type helper forCallDepthThreadLocalMapinstrumenter-module.md—instrumentationNames()version-aliasinstrumenter-module.md— preserve master's integration name when regeneratingadvice-class.md— single delegate method, not all overloadsadvice-class.md—onThrowablesemantics (exit-on-target-throw)docs/how_instrumentations_work.md:532-552advice-class.md— explicit charset when convertingbyte[]→StringDM_DEFAULT_ENCODINGadvice-class.md— noNullPointerExceptioncatchesDCN_NULLPOINTER_EXCEPTIONadvice-class.md—@AppliesOnfor multiple advicesdocs/how_instrumentations_work.mdtests.md— Java tests only, no new.groovyfilesEnforce Groovy Migrationworkflowtests.md— register names inmetadata/supported-configurations.jsoncheckInstrumenterModuleConfigurations+config-inversion-lintertests.md— cover error/exception scenariostests.md—compileOnlyvstestImplementationversion splittests.md— prior-version module intestImplementationmuzzle.md—assertInverse=truetraps + Pattern A/Bmuzzle.md— exclude incompatible major versionsmuzzle.md—skipVersionsfor malformed release versionsjedis-3.6.2Review feedback addressed
Round 1 of reviews (2026-07-08, from @mcculls and Copilot) surfaced 11 substantive issues. All addressed in
13d6bd32abandd4600a4624:SKILL.mdStep 4 source layout no longer contradicts Step 9.1 (src/test/java/throughout)naming-conventions.mddropped the inventedgRPCInstrumentationexample and the buggy sanity-check scripttests.mderror-test example now usesAgentSpan+span.getTag()(was OpenTelemetry'sSpanData)tests.mdsupported-configurations.jsontemplate default is now"true"(was"false")tests.mdno-groovy enforcement now references the real workflow (Enforce Groovy Migration), not a made-up bot namecontext-tracking.mdrxjava-2.0 subscribe reference softened to describe the actual matcher patternadvice-class.mdonThrowablesemantics rewritten perdocs/how_instrumentations_work.md:532-552(the earlier "SAFER" claim was factually wrong)advice-class.mdinline note added:StandardCharsetsisjava.nio.*and forbidden in bootstrap advicePaired 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.