skill(apm-integrations): add rules from recent reviews + refactor into sub references#11760
Open
jordan-wong wants to merge 22 commits into
Open
skill(apm-integrations): add rules from recent reviews + refactor into sub references#11760jordan-wong wants to merge 22 commits into
jordan-wong wants to merge 22 commits into
Conversation
Contributor
🟢 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. |
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.
mcculls
reviewed
Jul 8, 2026
Contributor
Author
|
For some context, how I create these prompt updates goes like:
Evaluation process: |
jordan-wong
added a commit
that referenced
this pull request
Jul 8, 2026
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>
Contributor
|
🎯 Code Coverage (details) 🔗 Commit SHA: 8394775 | Docs | Datadog PR Page | Give us feedback! |
jordan-wong
changed the title
jordan-wong
changed the title
Contributor
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![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".
PerfectSlayer
left a comment
PerfectSlayer
left a comment
Contributor
There was a problem hiding this comment.
Left minor comment about testing
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b8741c469
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…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>
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>
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
mcculls
force-pushed
the
skill/r13-r33-from-toolkit
branch
from
b5d44e0 to
8394775
Compare
Contributor
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
DDCI didn't respond in time. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds ~20 rules to the canonical
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)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.