fix: separate default vs controller-telemetry metadata test suites so instrumentation-list.yaml reflects out-of-the-box telemetry#18974
Conversation
… instrumentation-list.yaml reflects out-of-the-box telemetry Fixes open-telemetry#18369
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR restructures Gradle test tasks for several Java agent instrumentations to isolate “controller telemetry” test execution and metadata collection, and updates the generated instrumentation telemetry doc conditions accordingly.
Changes:
- Adds dedicated
testControllerTelemetry(and related) Gradle test tasks and wires them intocheck. - Excludes certain test packages from the default
testtask and runs them under specialized tasks with required JVM args/system properties. - Updates
docs/instrumentation-list.yamltelemetrywhen:conditions to separate default vs controller-telemetry-specific signals.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/build.gradle.kts | Adds controller-telemetry test task, excludes subsets from default test, updates task dependencies/finalizers. |
| instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/build.gradle.kts | Same refactor pattern for Spring WebMVC 3.1 module. |
| instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/build.gradle.kts | Splits server tests into controller-telemetry tasks; adds stable semconv + controller combos and finalizers. |
| instrumentation/ratpack/ratpack-1.4/javaagent/build.gradle.kts | Introduces controller-telemetry test task and adjusts stable semconv task metadata config. |
| docs/instrumentation-list.yaml | Adjusts telemetry condition blocks (default vs controller telemetry; stable semconv variants). |
| test { | ||
| exclude("**/boot/**", "**/filter/**") | ||
| } | ||
|
|
||
| val testControllerTelemetry by registering(Test::class) { | ||
| testClassesDirs = sourceSets.test.get().output.classesDirs | ||
| classpath = sourceSets.test.get().runtimeClasspath | ||
| jvmArgs("-Dotel.instrumentation.common.experimental.controller-telemetry.enabled=true") |
| test { | ||
| exclude("**/boot/**", "**/filter/**") | ||
| } | ||
|
|
||
| val testControllerTelemetry by registering(Test::class) { | ||
| testClassesDirs = sourceSets.test.get().output.classesDirs | ||
| classpath = sourceSets.test.get().runtimeClasspath | ||
| systemProperty( |
| test { | ||
| exclude("**/server/**") | ||
| } | ||
|
|
||
| val testControllerTelemetry by registering(Test::class) { | ||
| testClassesDirs = sourceSets.test.get().output.classesDirs | ||
| classpath = sourceSets.test.get().runtimeClasspath | ||
| include("**/server/**") | ||
| jvmArgs("-Dotel.instrumentation.common.experimental.controller-telemetry.enabled=true") |
| val testStableSemconv by registering(Test::class) { | ||
| testClassesDirs = sourceSets.test.get().output.classesDirs | ||
| classpath = sourceSets.test.get().runtimeClasspath | ||
| jvmArgs("-Dotel.instrumentation.common.experimental.controller-telemetry.enabled=true") | ||
| jvmArgs("-Dotel.semconv-stability.opt-in=service.peer") |
| val testExceptionSignalLogs by registering(Test::class) { | ||
| testClassesDirs = sourceSets.test.get().output.classesDirs | ||
| classpath = sourceSets.test.get().runtimeClasspath | ||
| jvmArgs("-Dotel.instrumentation.common.experimental.controller-telemetry.enabled=true") | ||
| jvmArgs("-Dotel.semconv.exception.signal.preview=logs") |
jaydeluca
left a comment
There was a problem hiding this comment.
Thanks for digging into this. After validating against the actual collected output, I think a couple of the modules called out in the issue don't actually need changes. For ratpack and the two webmvc modules, regenerating the metadata with the split produces an identical instrumentation-list.yaml because their own-scope telemetry is controller-only to begin with (the client/server spans are attributed to the netty/servlet scopes), so there's no default-vs-controller mislabeling to fix. We can drop those three and keep the change scoped to spring-webflux, which is the one module where the split actually moves telemetry between buckets. I'll update the issue with the findings.
| exclude("**/server/**") | ||
| } | ||
|
|
||
| val testControllerTelemetry by registering(Test::class) { |
There was a problem hiding this comment.
looking at the results of this, it doesn't change anything in the documented telemetry, so i think we can undo the file and consider ratpack as fine as-is.
| jvmArgs("-XX:+IgnoreUnrecognizedVMOptions") | ||
|
|
||
| systemProperty("collectMetadata", otelProps.collectMetadata) | ||
| } |
There was a problem hiding this comment.
this didn't result in any changes in the resulting telemetry, so we can revert all the changes in this file
| systemProperty("testLatestDeps", otelProps.testLatestDeps) | ||
| } | ||
|
|
||
| test { |
There was a problem hiding this comment.
this didn't result in any changes in the resulting telemetry, so we can revert all the changes in this file
|
|
||
| if (otelProps.collectMetadata) { | ||
| test { | ||
| finalizedBy(testControllerTelemetry) |
There was a problem hiding this comment.
this is an interesting approach, but at the moment we have a pattern of listing the tests in this file to add them to collection.
so instead of using this finalizedBy block we add spring:spring-webflux:spring-webflux-5.0:javaagent:testControllerTelemetry and spring:spring-webflux:spring-webflux-5.0:javaagent:testControllerTelemetryStableSemconv after line 281
| default: false | ||
| telemetry: | ||
| - when: otel.instrumentation.common.experimental.controller-telemetry.enabled=true | ||
| - when: default |
There was a problem hiding this comment.
you can also revert the changes to this file, and it will be updated automatically after this is merged
Pull request dashboard statusStatus last refreshed: 2026-07-23 05:18:23 UTC.
This automated status or its linked feedback items may be incorrect. If something looks wrong, please report it with the result you expected. If you believe this pull request is incorrectly routed as waiting on the author, comment |
…ist indentation Reverts the ratpack and both webmvc module changes (their own-scope telemetry is controller-only, so regenerated metadata is identical), trims the spring-webflux task wiring, adds the missing server exclude to testExceptionSignalLogs, and fixes the telemetry list indentation in the generated doc metadata.
|
Addressed the open feedback in 76a2a0b: reverted ratpack and both webmvc modules (their own-scope telemetry is controller-only, regeneration is identical), added the missing server exclude to testExceptionSignalLogs, and fixed the telemetry list indentation in the doc metadata. |
Summary
Mixed controller-telemetry modules now collect documentation metadata from a true default suite, so
docs/instrumentation-list.yamlstops attributing out-of-the-box telemetry (like WebFlux WebClient CLIENT spans andhttp.client.request.duration) to theotel.instrumentation.common.experimental.controller-telemetry.enabled=truecondition.Why this matters
#18369 reports that modules such as
spring-webflux-5.0run their whole test task with the controller-telemetry jvmArg and a flaggedmetadataConfig, so every signal the suite emits is documented as requiring the flag, including telemetry users get by default. The earlier parts of the issue (the missing=truesuffix and the webmvc controller spans in the default bucket) were already fixed on main; this addresses the remaining piece, the missing default suites in the mixed modules.Changes
spring-webflux-5.0,spring-webmvc-3.1,spring-webmvc-6.0, andratpack-1.4follow the existing named-suite pattern (testExperimental/testStableSemconvin the same files): the defaulttesttask drops the controller-telemetry jvmArg andmetadataConfig, and a dedicatedtestControllerTelemetrysuite carries the flag withmetadataConfig: otel.instrumentation.common.experimental.controller-telemetry.enabled=true.service.peermetadata decoupled from the controller flag, so the WebClient stable-semconv signals are not documented as controller-conditional.docs/instrumentation-list.yamlsections are updated to match the new suite split.Testing
Targeted suites for the four modules pass with the new split (default suites without the flag,
testControllerTelemetrysuites with it); combined-condition suites keep their existing assertions. A-PcollectMetadata=trueregeneration over the full ~36-module set is left to CI since the metadata collection run is repo-wide.Fixes #18369