[DO NOT MERGE] feat(sparkjava): Add sparkjava-gen-2.3 module (toolkit-generated)#11337
Draft
jordan-wong wants to merge 2 commits into
Draft
[DO NOT MERGE] feat(sparkjava): Add sparkjava-gen-2.3 module (toolkit-generated)#11337jordan-wong wants to merge 2 commits into
jordan-wong wants to merge 2 commits into
Conversation
Contributor
|
❌ New Groovy Files Detected Please avoid introducing new
Instead, rewrite the new file(s) in Java / JUnit. See the How to Test With JUnit Guide for more details. If this PR needs an exception, add the |
jordan-wong
changed the title
…allel to sparkjava-2.3)
This PR adds a new module `dd-java-agent/instrumentation/spark/sparkjava-gen-2.3`
containing an alternative SparkJava 2.3 instrumentation generated by the
APM instrumentation toolkit. Placed alongside existing `sparkjava-2.3` (not
replacing it) so reviewers can compare side by side.
Module name follows dd-trace-java's instrumentation-naming convention:
{framework}-gen-{version} so the name ends with the required version suffix
(initial attempt used sparkjava-2.3-gen which would have failed the
checkInstrumentationNaming Gradle task).
## What the toolkit did
One agent-driven workflow run produced the module end-to-end. Cost: $74.72,
duration ~2.5 hours, 10 reviewer iterations.
Key choices the agent made:
- Instruments `spark.route.Routes.find()` — matches the existing `sparkjava-2.3`
module's `RoutesInstrumentation` on `spark.route.Routes`
- Enriches the existing Jetty server span with `http.route` rather than creating
a new span (existing module follows the same approach; Jetty is always the
embedded server under SparkJava)
- Open-ended muzzle version range `[2.3,)` with `assertInverse = true`
- Spock tests extending `HttpServerTest`
## Verification
```
./gradlew :dd-java-agent:instrumentation:spark:sparkjava-gen-2.3:check \\
:dd-java-agent:instrumentation:spark:sparkjava-gen-2.3:muzzle \\
:dd-java-agent:instrumentation:spark:sparkjava-gen-2.3:latestDepTest
BUILD SUCCESSFUL in 46s
```
Multi-JVM CI matrix not run locally; standard CI will cover that.
## Reviewer notes
The toolkit's reviewer step flagged 7 TODOs across 10 iterations about
spec-compliance (suggesting `MatcherFilter.doFilter` as a target + creating
an own server span). The **existing** `sparkjava-2.3` module on master also
instruments `Routes` (not `MatcherFilter`) and relies on Jetty for span
creation — see `dd-java-agent/instrumentation/spark/sparkjava-2.3/src/main/java/datadog/trace/instrumentation/sparkjava/RoutesInstrumentation.java`.
The agent's implementation matches the canonical pattern. The reviewer's
suggestions appear to apply generic HTTP-server guidance that doesn't fit
this library's convention.
## Provenance
Generated by apm-instrumentation-toolkit (DataDog/apm-instrumentation-toolkit
branch eval/java). Research artifacts in the toolkit repo:
- docs/eval-research/runs/sparkjava/attempt3-final/
- docs/eval-research/hypotheses/sparkjava.md
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jordan-wong
force-pushed
the
eval/sparkjava-2.3-gen-toolkit-attempt
branch
from
7f8bfb4 to
11a3039
Compare
jordan-wong
changed the title
Contributor
@jordan-wong we disallow new groovy files. As per our policy, new tests have to be written in java |
amarziali
reviewed
May 13, 2026
amarziali
left a comment
amarziali
left a comment
Contributor
There was a problem hiding this comment.
@jordan-wong I'm interested in knowing the reason why this instrumentation has been added. Originally, this has been added to enrich the route but generally speaking a LLM can take different directions on what should be supported in terms of observability. I'm interested in knowing the actual prompt. Is it on the PR description?
| implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { | ||
|
|
||
| public SparkJavaInstrumentation() { | ||
| super("sparkjava", "sparkjava-2.4"); |
Contributor
There was a problem hiding this comment.
why 2.4 if the module is called 2.3? Interestingly there is the same typo in the current sparkjava instrumentation that let me think again that the implementation is not completely blind
4 tasks
This was referenced Jun 25, 2026
PerfectSlayer
added
the
tag: apm integration toolkit
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-cf854d Bot
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>
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
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
Adds a new module
dd-java-agent/instrumentation/spark/sparkjava-2.3-gencontaining an alternative SparkJava 2.3 instrumentation generated end-to-end by the APM instrumentation toolkit'snew_integrationworkflow.The new module is placed alongside the existing
sparkjava-2.3module (not replacing it) so reviewers can compare the toolkit-generated implementation against the canonical one side by side. This is a research artifact — the goal is to evaluate how well the toolkit can produce a Java instrumentation that passes CI without human intervention.What's in this PR
dd-java-agent/instrumentation/spark/sparkjava-2.3-gen/build.gradledd-java-agent/instrumentation/spark/sparkjava-2.3-gen/src/main/java/.../SparkJavaInstrumentation.javadd-java-agent/instrumentation/spark/sparkjava-2.3-gen/src/test/groovy/.../SparkJavaServer.groovydd-java-agent/instrumentation/spark/sparkjava-2.3-gen/src/test/groovy/.../SparkJavaTest.groovysettings.gradle.ktsNo changes to any production files or other modules.
How the toolkit produced this
One agent-driven workflow run from a blind baseline (the existing
sparkjava-2.3module was hidden from the agent). The workflow ran for ~2.5 hours, cost ~$74, and went through 10 reviewer iterations before the workflow exited.Key choices the agent made:
spark.route.Routes.find()— same instrumented type as the existingsparkjava-2.3module (which is namedRoutesInstrumentation)http.routerather than creating a new span. Matches existing pattern — Jetty is always the embedded server under SparkJava, so a server span always exists[2.3,)withassertInverse = trueHttpServerTestfor full server-span validationVerification
Local CI-equivalent verification on the new module:
./gradlew :dd-java-agent:instrumentation:spark:sparkjava-2.3-gen:check \ :dd-java-agent:instrumentation:spark:sparkjava-2.3-gen:muzzle \ :dd-java-agent:instrumentation:spark:sparkjava-2.3-gen:latestDepTest BUILD SUCCESSFUL in 46sMulti-JVM matrix (JVM 8/17/21/25) not run locally; the standard CI pipeline will exercise that.
Reviewer notes
The toolkit's internal reviewer step did NOT approve this module — it ran 10 iterations and exited with
todos_remaining=7. The recurring spec-compliance complaint was that the agent should have instrumentedspark.http.matching.MatcherFilter.doFilter(and created its own server span) instead of usingRoutes.find()+ Jetty span enrichment.I cross-referenced this against the existing
sparkjava-2.3module on master:dd-java-agent/instrumentation/spark/sparkjava-2.3/src/main/java/datadog/trace/instrumentation/sparkjava/RoutesInstrumentation.javainstrumentedType()returns"spark.route.Routes"startSpan— relies on Jetty's existing instrumentationSo the agent's choice matches the existing canonical pattern. The reviewer's spec-compliance critique appears to be applying generic HTTP-server guidance ("each framework should own its server span") that doesn't fit this library's established convention.
Reviewers familiar with SparkJava should evaluate whether the existing pattern is itself correct. If it is, this PR is functionally equivalent to the existing module and can be considered a successful toolkit reproduction. If the existing module should be refactored to create its own span, this PR has the same issue.
Test plan
:sparkjava-2.3-gen:testand:sparkjava-2.3-gen:forkedTest:sparkjava-2.3-gen:muzzle:sparkjava-2.3-gen:latestDepTestsparkjava-2.3module (the two should coexist; we may want to disable one or pick one before merging)Provenance
Research artifacts (agent transcript, hypothesis file, all skill changes the toolkit applied) are preserved in DataDog/apm-instrumentation-toolkit branch
eval/java:docs/eval-research/runs/sparkjava/attempt3-final/— lean snapshotdocs/eval-research/hypotheses/sparkjava.md— hypothesis file documenting the research findingThis PR is a draft for review only — not intended for merge as-is. The duplicate module would need to be reconciled (likely by replacing the existing
sparkjava-2.3with this one, or by withdrawing) before merging.🤖 Generated with Claude Code