Uh oh!
There was an error while loading. Please reload this page.
feat(feign-10.8): toolkit-generated feign HTTP client instrumentation - #11709
Open
jordan-wong wants to merge 4 commits into
Open
feat(feign-10.8): toolkit-generated feign HTTP client instrumentation#11709jordan-wong wants to merge 4 commits into
jordan-wong wants to merge 4 commits into
Conversation
Toolkit-generated greenfield instrumentation for Feign HTTP client v10.8+. Run details: - Branch (toolkit): eval/java @ 757979b9 - Branch (worktree): eval/feign-blind-attempt1-20260623 in /Users/jordan.wong/dd-trace-java-eval-feign - Workflow: new_integration, completed 2026-06-23 - Cost: $26.95, 78min, reviewer approved=True - Maven coordinates: io.github.openfeign:feign-core:10.8 Generated module: dd-java-agent/instrumentation/feign/feign-10.8-generated/ - FeignClientInstrumentation (sync Client.execute) - FeignAsyncClientInstrumentation (async AsyncClient.execute) - FeignClientDecorator (HttpClientDecorator) - RequestHeaderInjectAdapter (header injection for distributed tracing) - SpanFinishingCallback (async future completion) - Java tests (FeignClientTest, FeignAsyncClientTest) per R20 Supersedes #10855 (Feign 8.0, stale) and closed#10980 (Feign 10.8, never independently verified). ## Research integrity note `metadata/supported-configurations.json` (4 entries: DD_TRACE_FEIGN_10_8_ENABLED, DD_TRACE_FEIGN_ANALYTICS_ENABLED, DD_TRACE_FEIGN_ANALYTICS_SAMPLE_RATE, DD_TRACE_FEIGN_ENABLED) was **hand-added** by the human operator AFTER the toolkit run; the toolkit's R29 rule was added later in the same session (see commit 757979b9 on eval/java). This is documented in docs/eval-research/generated/feign-20260623/caveats.md. `settings.gradle.kts` line addition is a faithful replay of what the toolkit produced in the eval worktree. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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. |
… name The checkDecoratorAnalyticsConfigurations task requires ANALYTICS entries for each instrumentation name returned by Decorator.instrumentationNames(), not just the shared name. FeignClientDecorator returns ['feign', 'feign-10.8']; the 'feign' analytics keys were already added, but 'feign-10.8' was missing. Adds: - DD_TRACE_FEIGN_10_8_ANALYTICS_ENABLED - DD_TRACE_FEIGN_10_8_ANALYTICS_SAMPLE_RATE (type: decimal) Locally verified ./gradlew checkConfigurations BUILD SUCCESSFUL. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
R31 — drop assertInverse from muzzle: versions = '[10.8,)' with assertInverse = true was failing because feign 10.6.0 actually passes muzzle (FeignClientInstrumentation sync class doesn't need 10.8 features; only FeignAsyncClientInstrumentation does). assertInverse asserts inverse versions FAIL muzzle — but 10.6.0 PASSES, so the auto-test failed. Conservative omission of assertInverse is correct here. The min version remains 10.8 (compileOnly + muzzle pass range). Fixes: muzzle: [2/8] CI failure (muzzle-AssertFail-io.github.openfeign-feign-core-10.6.0). R32 — '-generated' exclusion: Added 'feign-10.8-generated' to instrumentationNaming.exclusions in dd-java-agent/instrumentation/build.gradle. Fixes check-instrumentation-naming. Local verification: ./gradlew :dd-java-agent:instrumentation:feign:feign-10.8-generated:muzzle BUILD SUCCESSFUL ./gradlew checkInstrumentationNaming BUILD SUCCESSFUL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Spotless moved the trailing comment 'org-json does not use semver' to its own line. Fixes dd-gitlab/spotless CI failure introduced by the R32 edit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jordan-wong
commented
Jun 24, 2026
ContributorAuthor
Only fails CI check dd-gitlab/validate_supported_configurations_v2_local_file because of config keys arent registered as this is a completely new instrumentation. We can assume CI checks are passing |
jordan-wong
marked this pull request as ready for review
June 25, 2026 00:36
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:1a6fd6d284
ℹ️ 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".
| } | ||
| if (future != null) { | ||
| future = future.whenComplete(new SpanFinishingCallback(span)); |
There was a problem hiding this comment.
When an AsyncClient implementation returns a cancellable CompletableFuture (for example Feign's async clients use cancellation to abort in-flight requests), replacing it with the dependent stage from whenComplete breaks that contract: cancelling the future returned to user code only cancels the dependent stage and leaves the original request running. Register the callback without assigning it back, or otherwise propagate cancellation to the original future.
Useful? React with 👍 / 👎.
| implements Instrumenter.ForTypeHierarchy, Instrumenter.HasMethodAdvice { | ||
| public FeignClientInstrumentation() { | ||
| super("feign", "feign-10.8"); |
There was a problem hiding this comment.
This generated/eval module is documented in the new metadata as disabled by default, but InstrumenterModule.Tracing uses defaultEnabled() from the base class unless it is overridden, so this constructor enables the sync and async Feign instrumentations for every Feign 10.8+ application by default. If this module is meant to be opt-in, override defaultEnabled() in both instrumentation classes; otherwise the metadata default is misleading and users will get spans unless they explicitly opt out.
Useful? React with 👍 / 👎.
This was referenced Jun 25, 2026
| implements Instrumenter.ForTypeHierarchy, Instrumenter.HasMethodAdvice { | ||
| public FeignAsyncClientInstrumentation() { | ||
| super("feign", "feign-10.8"); |
Contributor
There was a problem hiding this comment.
Since there's only one Feign module, the second name is redundant - super("feign") is enough. Each name here becomes its own enable flag (DD_TRACE_FEIGN_ENABLED + DD_TRACE_FEIGN_10_8_ENABLED), so the extra name just doubles the config surface for no gain. The group-name + versioned-name form (like super("okhttp", "okhttp-3")) is only needed when multiple version-specific modules exist and we want one shared switch across them.
| { | ||
| "version": "A", | ||
| "type": "boolean", | ||
| "default": "false", |
Contributor
There was a problem hiding this comment.
All our HTTP client integrations default to true (see e.g. DD_TRACE_OKHTTP_ENABLED), so this should be true as well. This field must also match the code: the module doesn't override defaultEnabled(), so it's actually enabled by default - false here is inconsistent. If we do want Feign off by default, override defaultEnabled() to return false in both instrumentation classes.
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static AgentScope methodEnter( | ||
| @Advice.Argument(value = 0, readOnly = false) Request request) { | ||
| final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(AsyncClient.class); |
Contributor
There was a problem hiding this comment.
Instrumenting both the async and sync layers produces two spans per request, and call-depth can't reliably dedupe them.
AsyncClient.Pseudo delegates synchronously to Client.Default, which FeignClientInstrumentation also instruments. Since async increments AsyncClient.class and sync increments Client.class, the counters are independent - the inner Client.execute still sees depth 0 and starts its own span.
Since every Feign call ultimately goes through feign.Client.execute, the cleaner design is to instrument a single layer - most likely just the sync Client - giving exactly one span in all cases except a fully custom async client that bypasses feign.Client.
mcculls pushed a commit
that referenced
this pull request
Jul 9, 2026
…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>
gh-worker-dd-mergequeue-cf854dBot
pushed a commit
that referenced
this pull request
Jul 9, 2026
…o sub references (#11760) skill(add-apm-integrations): R13-R33 + Cat B (context-propagation) Step 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> skill(add-apm-integrations): clean up R-numbering and sub-step naming 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> refactor(skill): extract Category B routing to references/ 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> refactor(skill): extract naming conventions to references/ 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> refactor(skill): extract InstrumenterModule guidance to references/ 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> refactor(skill): extract Advice class guidance to references/ 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> refactor(skill): extract tests guidance to references/ 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> refactor(skill): extract muzzle guidance to references/ 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> style(skill): add missing blank lines before Step/subsection headings 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> refactor(skill): drop toolkit-internal 'Category A/B' language 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> refactor(skill): remove toolkit-workflow language from reference files 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> fix(skill): address review comments on #11760 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> fix(skill): trim bootstrap note on charset to a single line 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> review feedback Fix docs inconsistencies Rename skill to apm-integrations remove redundant note Renamed .claude/skills to .agents/skills and added symlink-style redirects for the old location Align NIO docs advice with skill Simplify SKILL Fix constructor advice best-practice Address codex comments Co-authored-by: mcculls <stuart.mcculloch@datadoghq.com> Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
gh-worker-dd-mergequeue-cf854dBot
pushed a commit
that referenced
this pull request
Jul 16, 2026
…R 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>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Toolkit-generated greenfield instrumentation for Feign HTTP client (io.github.openfeign:feign-core 10.8+). Part of the dd-trace-java APM toolkit eval program.⚠️ DO NOT MERGE. Open for review;
reviewer feedback feeds back into toolkit R-rules.
Wins from prior reviewer feedback
@mcculls reviewed the prior toolkit feign attempt on #10579 (CLOSED). This regeneration addresses three of his key concerns:
## Regressions / concerns vs prior review and master conventions
asked for the ReactorCoreModule pattern explicitly on DRAFT: APM AI TOOLKIT New Integration: Feign Client Instrumentation #10579.
keys → two HTTP spans per request. Tests assert this rather than fix it. Use a shared sentinel.
What was structurally correct
HttpClientDecorator<Request, Response> shape matches master HTTP-client conventions. Module path dd-java-agent/instrumentation/feign/feign-10.8-generated/ follows version-suffix + parent-dir
naming.
CI
All substantive jobs pass (build, instrumentationTest, latestDepTest, muzzle 8/8, smokeTest, profiling). Only failing check is dd-gitlab/validate_supported_configurations_v2_local_file —
external registry validator requires DD_TRACE_FEIGN_* keys to be pre-registered out-of-band. Not a code issue.
What reviewers should do
Related
Prior toolkit attempts: #10579 (review from @mcculls), #10980, #10855. Sibling eval PRs: #11708 (sparkjava), #11717 (commons-httpclient).
🤖 Generated with Claude Code