feat(commons-httpclient-2.0): toolkit-generated regeneration by jordan-wong · Pull Request #11717 · DataDog/dd-trace-java

jordan-wong · 2026-06-23T19:18:56Z

Summary

Toolkit-generated regeneration of dd-java-agent/instrumentation/commons-httpclient-2.0/. Part of the dd-trace-java APM toolkit eval program. ⚠️ DO NOT MERGE. Open for review.

What was preserved or restored from master

IastHttpMethodBaseInstrumentation.java — separate @autoservice IAST module (toolkit only generates tracing; IAST plumbing restored verbatim from master)
CommonsHttpClientDecorator.sourceUrl(HttpMethod) override (required for IAST SSRF chain)
commons-http-client-x muzzle directive
## Wins from prior reviewer feedback

@PerfectSlayer reviewed the prior toolkit commons-httpclient attempt on #10941 (still open, stale). This regeneration addresses every concrete concern:

✅ Single delegate matcher — instruments only the 3-arg executeMethod(HostConfiguration, HttpMethod, HttpState). Other overloads delegate to this one, so no duplicate spans / no duplicate
header injection. (Was @PerfectSlayer's central concern.)
✅ Muzzle bounded [2.0,4.0) + explicit fail [,2.0) — answers "Do you know it excluded version 4?"
✅ latestDepTestImplementation 2+ — answers "It should be 2+"
✅ Component name "commons-http-client" consistent across instrumenter, decorator, and test (this consistency bug caused 28 test failures earlier in the session, now fixed)
✅ status() uses statusLine == null ? 0 : statusLine.getStatusCode() (no catch(NullPointerException))
✅ JUnit 5, no Groovy

Regressions vs master — surfaced by diff-vs-master pass

Three losses from the toolkit's HelperMethods refactor that no prior reviewer flagged:

CommonsHttpClientDecorator.url() (lines 34-45) catches generic Exception and returns null. Master narrowed to URIException and rethrew URISyntaxException — propagates typed signal to the
base decorator. Current code silently swallows; malformed URIs become invisible.
Lost BlockingException re-throw path. Master's ExecAdvice had explicit catch (BlockingException e) + call-depth reset + rethrow for AppSec blocking. Current HelperMethods.doMethodEnter has
no such branch; suppress = Throwable.class will swallow it — AppSec-initiated blocks won't propagate.
Lost @AppliesOn(CONTEXT_TRACKING) advice gate. Master split work across ExecAdvice + a separate ContextPropagationAdvice annotated with @AppliesOn(CONTEXT_TRACKING). Current code collapses
into one advice and inlines injectContext — drops the target-system marker.

Other notes

Test class doesn't extend shared HttpClientTest base — loses cross-tracer behavioral parity. 615 lines of standalone JUnit 5 vs master's Groovy spec inheriting the shared suite.
skipVersions += "20020423" removed from muzzle — could cause failures on that ancient artifact. Cheap to re-add.
Complex commit history (toolkit gen → component-name fix → master-deletion → restructure → master merge → spotless → IAST restore). Review git diff master...HEAD rather than commit-by-commit.

CI

Pipeline triggered by 2ff02f4 (IAST restoration). At time of writing: 580 pass / 0 pending / 1 fail. The 16 IAST SSRF smoke tests that previously failed (because the toolkit's overwrite
deleted IastHttpMethodBaseInstrumentation.java) are now passing again.

What reviewers should do

Address the three diff-vs-master regressions: url() exception narrowing, BlockingException path, @AppliesOn(CONTEXT_TRACKING) gate.
Decide whether to port tests to extend HttpClientTest base.
The three regressions reveal a refactor-preservation failure mode in the toolkit — when extracting advice into a helper class, the toolkit dropped existing catch branches and annotations.
Worth encoding as an R-rule.

…OT MERGE] Toolkit-generated regeneration of commons-httpclient-2.0 instrumentation, placed side-by-side with the existing master module via the -generated suffix convention. Supersedes #10941 (PerfectSlayer's duplicate-module review). Run details: - Toolkit branch: eval/java @ 757979b9+ - Workflow: new_integration (default) - Maven coordinates: commons-httpclient:commons-httpclient:2.0.2 - Cost: $31.91 - Duration: 119 min - Reviewer verdict: approved=True, todos_fixed=0, todos_remaining=0 Generated module: dd-java-agent/instrumentation/commons-httpclient/commons-httpclient-2.0-generated/ - CommonsHttpClientInstrumentation (HttpMethodBase.execute) - CommonsHttpClientDecorator (HttpClientDecorator) - HttpHeadersInjectAdapter (header injection) - HelperMethods - Java tests (per R20, no Groovy) ## Research integrity disclosure Hand-added by human operator AFTER toolkit run (toolkit did not produce these): - 6 entries in metadata/supported-configurations.json: DD_TRACE_COMMONS_HTTPCLIENT_ENABLED, DD_TRACE_COMMONS_HTTPCLIENT_2_0_ENABLED, and their ANALYTICS_ENABLED + ANALYTICS_SAMPLE_RATE pairs (with type 'decimal' per R29). - 1 line in settings.gradle.kts to register the new module. Research signal — integration name divergence: master's deleted module used super('commons-http-client') (with dashes); toolkit-generated module uses super('commons-httpclient', 'commons-httpclient-2.0'). The R29 rule encoded earlier today doesn't yet require matching existing master conventions when regenerating; that's a research gap to encode. Full caveats: docs/eval-research/generated/commons-httpclient-20260623/caveats.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dd-octo-sts · 2026-06-23T19:43:52Z

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite	Status
Startup	🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results

Scenario	Candidate	master	Δ (95% CI of mean)
startup:insecure-bank:iast:Agent	14.00 s	13.90 s	[-0.0%; +1.4%] (no difference)
startup:insecure-bank:tracing:Agent	12.97 s	12.98 s	[-0.9%; +0.8%] (no difference)
startup:petclinic:appsec:Agent	16.91 s	16.74 s	[-0.1%; +2.1%] (no difference)
startup:petclinic:iast:Agent	16.88 s	17.01 s	[-1.6%; +0.0%] (no difference)
startup:petclinic:profiling:Agent	16.76 s	16.92 s	[-2.0%; +0.0%] (no difference)
startup:petclinic:sca:Agent	16.97 s	16.73 s	[+0.6%; +2.2%] (maybe worse)
startup:petclinic:tracing:Agent	16.03 s	16.11 s	[-1.4%; +0.4%] (no difference)

Commit: 2ff02f41 · CI Pipeline · Benchmarking Platform UI

Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

…ng exclusion) + R33 (no NPE catch) R30 — Preserve master's integration name convention: super('commons-httpclient', 'commons-httpclient-2.0') → super('commons-http-client') instrumentationNames() returns ['commons-http-client'] (matches master) UTF8BytesString.create value already used 'commons-httpclient' in toolkit output; align to 'commons-http-client' Removes 6 hand-added DD_TRACE_COMMONS_HTTPCLIENT_* entries from metadata/supported-configurations.json — master's existing DD_TRACE_COMMONS_HTTP_CLIENT_* entries cover the canonical name. Fixes: validate_supported_configurations_v2_local_file CI failure (entries weren't in central feature-parity registry because they were invented names). R32 — '-generated' exclusion: Added 'commons-httpclient-2.0-generated' to instrumentationNaming.exclusions in dd-java-agent/instrumentation/build.gradle. Fixes check-instrumentation-naming. R33 — No NPE catch in CommonsHttpClientDecorator.status(): Replaced try { httpMethod.getStatusCode() } catch (NullPointerException) with the canonical null-check guard: final StatusLine statusLine = httpMethod.getStatusLine(); return statusLine == null ? 0 : statusLine.getStatusCode(); Imported org.apache.commons.httpclient.StatusLine. Matches master's commons-httpclient-2.0 module. Fixes spotbugsMain DCN_NULLPOINTER_EXCEPTION. Local verification (all BUILD SUCCESSFUL): ./gradlew :dd-java-agent:instrumentation:commons-httpclient:commons-httpclient-2.0-generated:spotbugsMain ./gradlew checkConfigurations ./gradlew checkInstrumentationNaming 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>

…HttpClientTest The R30 fix (preserve master's super('commons-http-client') convention) was WORSE for runtime tests: both master's existing commons-httpclient-2.0/ AND the toolkit-generated commons-httpclient-2.0-generated/ then declared the same integration name AND the same instrumentedType, causing a runtime conflict where the generated module's instrumentation didn't load. Diagnosed via Datadog MCP log fetch of CI job 1798409915: 37 tests completed, 28 failed CommonsHttpClientTest > 7 test methods × 4 retries = 28 failures Restoring toolkit's original output: - super('commons-httpclient', 'commons-httpclient-2.0') in CommonsHttpClientInstrumentation - instrumentationNames returns ['commons-httpclient', 'commons-httpclient-2.0'] - COMMONS_HTTP_CLIENT constant uses 'commons-httpclient' - Re-add 6 DD_TRACE_COMMONS_HTTPCLIENT_* entries to supported-configurations.json R33 NPE-catch fix preserved. R32 naming exclusion preserved. The 'validate_supported_configurations_v2_local_file' CI check will fail again until the new DD_TRACE_COMMONS_HTTPCLIENT_* entries are registered in the central feature-parity registry (out-of-band Datadog operation). Research finding: the toolkit's '-generated' side-by-side convention is incompatible with R30 when regenerating an existing module — preserving master's integration name causes a runtime conflict because both modules declare the same name AND instrumented type. The convention needs to either invent new names (registry work) or remove master's settings.gradle.kts entry (modifies master, beyond eval scope). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…v2_local_file Building on JMS v3 PR #11596's pattern (passes v2_local_file by preserving master integration names): apply R30 to commons-httpclient WITHOUT the runtime collision risk that broke the previous attempt. Previous R30 attempt failed because master's commons-httpclient-2.0/ AND the generated commons-httpclient-2.0-generated/ both declared super('commons-http-client') AND instrumented the same HttpClient type — runtime conflict (28 of 37 tests failed). This time, ALSO remove master's commons-httpclient-2.0 from settings.gradle.kts so only the generated module loads. Acceptable for this [DO NOT MERGE] research PR — explicit supersede of master's module. Changes: - super('commons-http-client') in CommonsHttpClientInstrumentation - instrumentationNames returns ['commons-http-client'] - UTF8BytesString.create('commons-http-client') in COMMONS_HTTP_CLIENT - Remove 6 DD_TRACE_COMMONS_HTTPCLIENT_* hand-added entries - Remove ':dd-java-agent:instrumentation:commons-httpclient-2.0' from settings.gradle.kts Local verification BUILD SUCCESSFUL: muzzle, spotbugsMain, checkConfigurations, checkInstrumentationNaming, spotlessCheck. Expected to unblock validate_supported_configurations_v2_local_file — the central feature-parity registry already has DD_TRACE_COMMONS_HTTP_CLIENT_* from master. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Previous attempt only removed settings.gradle.kts entry; master module's source files were still tracked. At runtime, agent still picked up master's CommonsHttpClientInstrumentation alongside the generated module's instrumentation (same integration name), causing generated module's hooks to be shadowed. Result: 28 of 37 CommonsHttpClientTest tests failed (no spans). Solution: git rm -rf dd-java-agent/instrumentation/commons-httpclient-2.0/. Only commons-httpclient-2.0-generated/ remains. Files removed (566 lines): build.gradle, gradle.lockfile, source classes, and Groovy tests. Local verification BUILD SUCCESSFUL: checkConfigurations, muzzle, checkInstrumentationNaming. Deliberate [DO NOT MERGE] research-PR action: only ONE module per integration name at runtime. Reviewers: when merged, drop -generated suffix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tch Decorator The toolkit-generated test asserted span tag component='commons-httpclient' (no hyphen between http and client) while the toolkit-generated Decorator emits component='commons-http-client' (with hyphen). The mismatch caused all 7 tests that go through assertHttpClientSpan to fail in CI with 28 reported failures (7 unique × 4 Spock retries). Aligns the test with the Decorator's value, which matches the master convention (commons-http-client). All 16 tests now pass locally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…-generated suffix The parallel -generated/ directory caused (a) runtime double-instrumentation collision against the existing master commons-httpclient-2.0 module, and (b) when master was deleted to resolve (a), broke IAST's SSRF detection hook which had a hardcoded reference to the master path. Move the toolkit's regenerated files to the canonical master path (dd-java-agent/instrumentation/commons-httpclient-2.0/). Standard PR review uses git diff against origin/master to inspect what the toolkit produced; the parallel-directory convention was not actually solving a real reviewer need. Restores IAST SSRF coverage and eliminates the runtime collision. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ient-2.0-toolkit-attempt # Conflicts: # dd-java-agent/instrumentation/commons-httpclient-2.0/gradle.lockfile

The c661f9d restructure (removing commons-httpclient-2.0-generated from instrumentationNaming.exclusions) left the list in a layout spotless's Groovy formatter rejects. Auto-reformat to canonical style. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

The toolkit's new_integration workflow only generates APM tracing code, not IAST. The c661f9d restructure overwrote master's commons-httpclient-2.0/ with toolkit output, accidentally deleting master's IAST artifacts: - IastHttpMethodBaseInstrumentation.java: separate @autoservice module that taints HttpMethodBase constructor URI args for SSRF detection. Restored verbatim from origin/master. - CommonsHttpClientDecorator.sourceUrl(HttpMethod) override: used by HttpClientDecorator's URL-source pathway that IAST SSRF depends on. Added back to the toolkit's decorator. - build.gradle muzzle directive 'commons-http-client-x': declared by the IAST instrumenter (via muzzleDirective() method); restored. The toolkit-generated tracing files (Instrumentation, Decorator's other methods, helpers, tests) are left as the toolkit generated them — this commit only restores the non-tracing pieces master owned that the toolkit doesn't know about. Fixes the 16 IastSpringBootSmokeTest > "ssrf is present (commons-httpclient2) (url)" failures across JDK shards. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dd-octo-sts · 2026-06-25T04:16:55Z

Hi! 👋 Thanks for your pull request! 🎉

To help us review it, please make sure to:

Add at least one type, and one component or instrumentation label to the pull request

If you need help, please check our contributing guidelines.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ff02f4108

ℹ️ 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".

chatgpt-codex-connector · 2026-06-25T04:21:13Z

+    final AgentSpan span =
+        startSpan(CommonsHttpClientDecorator.COMMONS_HTTP_CLIENT.toString(), HTTP_REQUEST);
+    DECORATE.afterStart(span);
+    DECORATE.onRequest(span, method);


Restore BlockingException handling around onRequest

When AppSec/RASP blocks an outbound URL, DECORATE.onRequest can throw BlockingException; this advice is still declared with suppress = Throwable, so without the previous catch/reset path the exception is swallowed by the advice wrapper and CallDepthThreadLocalMap remains incremented. In that AppSec blocking scenario the commons-httpclient request is allowed to continue, and later calls on the same thread return null from doMethodEnter, disabling tracing for this client until the thread is reused/reset.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-06-25T04:21:13Z

+
+    final AgentScope scope = activateSpan(span);
+
+    DECORATE.injectContext(current(), method, SETTER);


Restore the context-tracking propagation advice

This injects propagation only from the tracing helper, whereas the previous ContextPropagationAdvice was a separate @AppliesOn(CONTEXT_TRACKING) advice. Since AgentInstaller enables CONTEXT_TRACKING independently of tracing, deployments with tracing disabled still used to forward existing trace context through commons-httpclient; after this collapse there is no context-tracking-only advice for this matcher, so those calls stop carrying propagation headers.

Useful? React with 👍 / 👎.

ygree · 2026-07-01T20:58:14Z

  @Override
  public void methodAdvice(MethodTransformer transformer) {
-    transformer.applyAdvices(
+    // All executeMethod overloads delegate to the 3-arg version:


This must be verified for each version to make sure it alway such three arguments because of it's much stricter and Java allows method overloading. Restricting arguments other than the one that's used seems to be too limiting. That can cause inflexibility if in some versions of the library this method signature changes.
The comment about avoiding duplicate spans is misleading. It's avoided anyways with the call-depth check, that is kept as-is anyways, but moved to a helper class.

ygree · 2026-07-01T20:58:56Z

-      }
+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    public static AgentScope methodEnter(@Advice.Argument(1) final HttpMethod method) {
+      return HelperMethods.doMethodEnter(method);


There is no value in introducing the HelperMethods class

…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>

@mcculls

…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>

jordan-wong and others added 10 commits June 23, 2026 21:42

fix(build): spotlessApply on instrumentation/build.gradle

73ce2ea

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>

Merge remote-tracking branch 'origin/master' into eval/commons-httpcl…

725f6ee

…ient-2.0-toolkit-attempt # Conflicts: # dd-java-agent/instrumentation/commons-httpclient-2.0/gradle.lockfile

This was referenced Jun 25, 2026

feat(sparkjava-2.3): toolkit-generated sparkjava-2.3 #11708

Open

feat(feign-10.8): toolkit-generated feign HTTP client instrumentation #11709

Open

jordan-wong added the tag: do not merge Do not merge changes label Jun 25, 2026

jordan-wong marked this pull request as ready for review June 25, 2026 04:16

jordan-wong requested review from a team as code owners June 25, 2026 04:16

jordan-wong requested review from claponcet, manuel-alvarez-alvarez and vandonr and removed request for a team June 25, 2026 04:16

dd-octo-sts Bot added the tag: ai generated Largely based on code generated by an AI or LLM label Jun 25, 2026

jordan-wong removed request for a team, claponcet, manuel-alvarez-alvarez and vandonr June 25, 2026 04:17

chatgpt-codex-connector Bot reviewed Jun 25, 2026

View reviewed changes

jordan-wong mentioned this pull request Jun 29, 2026

skill(apm-integrations): add rules from recent reviews + refactor into sub references #11760

Merged

jordan-wong changed the title ~~feat(commons-httpclient-2.0): toolkit-generated regeneration [DO NOT MERGE]~~ feat(commons-httpclient-2.0): toolkit-generated regeneration Jun 29, 2026

jordan-wong added inst: others All other instrumentations type: feature Enhancements and improvements labels Jun 29, 2026

amarziali requested review from a team and ygree and removed request for a team July 1, 2026 06:01

PerfectSlayer added the tag: apm integration toolkit Changes generated by DataDog/apm-instrumentation-toolkit as part of IDM AIT experimentation label Jul 1, 2026

ygree reviewed Jul 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(commons-httpclient-2.0): toolkit-generated regeneration#11717

feat(commons-httpclient-2.0): toolkit-generated regeneration#11717
jordan-wong wants to merge 11 commits into
masterfrom
eval/commons-httpclient-2.0-toolkit-attempt

jordan-wong commented Jun 23, 2026 •

edited

Loading

Uh oh!

dd-octo-sts Bot commented Jun 23, 2026 •

edited

Loading

Uh oh!

dd-octo-sts Bot commented Jun 25, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jun 25, 2026

Uh oh!

chatgpt-codex-connector Bot Jun 25, 2026

Uh oh!

ygree Jul 1, 2026

Uh oh!

ygree Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		final AgentScope scope = activateSpan(span);

		DECORATE.injectContext(current(), method, SETTER);

Uh oh!

Conversation

jordan-wong commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What was preserved or restored from master

Regressions vs master — surfaced by diff-vs-master pass

Other notes

CI

What reviewers should do

Related

Uh oh!

dd-octo-sts Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 Java Benchmark SLOs — All performance SLOs passed

Uh oh!

dd-octo-sts Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

ygree Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

ygree Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jordan-wong commented Jun 23, 2026 •

edited

Loading

dd-octo-sts Bot commented Jun 23, 2026 •

edited

Loading

dd-octo-sts Bot commented Jun 25, 2026 •

edited

Loading