feat(commons-httpclient-2.0): toolkit-generated regeneration [DO NOT MERGE]#11717
Draft
jordan-wong wants to merge 8 commits into
Draft
feat(commons-httpclient-2.0): toolkit-generated regeneration [DO NOT MERGE]#11717jordan-wong wants to merge 8 commits into
jordan-wong wants to merge 8 commits into
Conversation
…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>
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. |
…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>
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
1 participant
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 regeneration of
commons-httpclient-2.0instrumentation, placed side-by-side with the existing master module via the-generatedsuffix convention. Supersedes #10941 (PerfectSlayer's duplicate-module review from March 2026).Generated module
dd-java-agent/instrumentation/commons-httpclient/commons-httpclient-2.0-generated/CommonsHttpClientInstrumentation(HttpMethodBase.execute)CommonsHttpClientDecoratorextendsHttpClientDecoratorHttpHeadersInjectAdapter(header injection for distributed tracing)HelperMethodsRun details
eval/java@757979b9+new_integration(default)commons-httpclient:commons-httpclient:2.0.2Research integrity disclosure
Hand-added by human operator AFTER toolkit run (toolkit did not produce these):
metadata/supported-configurations.json:DD_TRACE_COMMONS_HTTPCLIENT_ENABLED,DD_TRACE_COMMONS_HTTPCLIENT_2_0_ENABLED, and theirANALYTICS_ENABLED+ANALYTICS_SAMPLE_RATEpairs (withtype: "decimal"per R29 today).settings.gradle.ktsto register the new module.Research signal — toolkit integration name divergence
Master's deleted module declared
super("commons-http-client")(integration namecommons-http-client, with dashes splitting "http" and "client"). The correspondingDD_TRACE_COMMONS_HTTP_CLIENT_*entries already exist in master.The toolkit-generated module declares
super("commons-httpclient", "commons-httpclient-2.0")— different integration names (no inner dash). The existing master metadata entries don't match these names, so I had to hand-add new entries.This is a research gap in R29: when regenerating an existing module, the toolkit should preserve the existing master conventions for integration names. Encoded as a finding in the bundle's caveats.md.
Caveats
Full disclosure:
docs/eval-research/generated/commons-httpclient-20260623/caveats.mdoneval/javabranch.Known expected CI failures
dd-gitlab/check-instrumentation-naming— expected fail due to-generatedsuffix research convention (same as JMS v3 feat(jms): toolkit-generated JMS 1.1 instrumentation v3 [DO NOT MERGE] #11596, feign feat(feign-10.8): toolkit-generated feign HTTP client instrumentation [DO NOT MERGE] #11709)dd-gitlab/muzzle: [N/M](possible) — if muzzle assertInverse hits a boundary version (same toolkit gap as feign hit on 10.6.0)dd-gitlab/validate_supported_configurations_v2_local_file(possible) — external validator we can't currently access logs for🤖 Generated with Claude Code