Skip to content

fix: separate default vs controller-telemetry metadata test suites so instrumentation-list.yaml reflects out-of-the-box telemetry#18974

Open
mvanhorn wants to merge 4 commits into
open-telemetry:mainfrom
mvanhorn:fix/18369-controller-telemetry-metadata
Open

fix: separate default vs controller-telemetry metadata test suites so instrumentation-list.yaml reflects out-of-the-box telemetry#18974
mvanhorn wants to merge 4 commits into
open-telemetry:mainfrom
mvanhorn:fix/18369-controller-telemetry-metadata

Conversation

@mvanhorn

Copy link
Copy Markdown

Summary

Mixed controller-telemetry modules now collect documentation metadata from a true default suite, so docs/instrumentation-list.yaml stops attributing out-of-the-box telemetry (like WebFlux WebClient CLIENT spans and http.client.request.duration) to the otel.instrumentation.common.experimental.controller-telemetry.enabled=true condition.

Why this matters

#18369 reports that modules such as spring-webflux-5.0 run their whole test task with the controller-telemetry jvmArg and a flagged metadataConfig, 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 =true suffix 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, and ratpack-1.4 follow the existing named-suite pattern (testExperimental / testStableSemconv in the same files): the default test task drops the controller-telemetry jvmArg and metadataConfig, and a dedicated testControllerTelemetry suite carries the flag with metadataConfig: otel.instrumentation.common.experimental.controller-telemetry.enabled=true.
  • The stable-semconv variant keeps service.peer metadata decoupled from the controller flag, so the WebClient stable-semconv signals are not documented as controller-conditional.
  • The touched docs/instrumentation-list.yaml sections 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, testControllerTelemetry suites with it); combined-condition suites keep their existing assertions. A -PcollectMetadata=true regeneration over the full ~36-module set is left to CI since the metadata collection run is repo-wide.

Fixes #18369

Copilot AI review requested due to automatic review settings June 12, 2026 08:19
@mvanhorn
mvanhorn requested a review from a team as a code owner June 12, 2026 08:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 into check.
  • Excludes certain test packages from the default test task and runs them under specialized tasks with required JVM args/system properties.
  • Updates docs/instrumentation-list.yaml telemetry when: 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).

Comment on lines 53 to 60
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")
Comment on lines 54 to 61
test {
exclude("**/boot/**", "**/filter/**")
}

val testControllerTelemetry by registering(Test::class) {
testClassesDirs = sourceSets.test.get().output.classesDirs
classpath = sourceSets.test.get().runtimeClasspath
systemProperty(
Comment on lines +60 to +68
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")
Comment on lines 75 to 79
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")
Comment on lines 116 to 120
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")
Comment thread docs/instrumentation-list.yaml
Comment thread docs/instrumentation-list.yaml Outdated
Comment thread docs/instrumentation-list.yaml Outdated
Comment thread docs/instrumentation-list.yaml Outdated

@jaydeluca jaydeluca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs/instrumentation-list.yaml
Comment thread docs/instrumentation-list.yaml Outdated
exclude("**/server/**")
}

val testControllerTelemetry by registering(Test::class) {

@jaydeluca jaydeluca Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread docs/instrumentation-list.yaml Outdated
default: false
telemetry:
- when: otel.instrumentation.common.experimental.controller-telemetry.enabled=true
- when: default

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can also revert the changes to this file, and it will be updated automatically after this is merged

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 18, 2026

Copy link
Copy Markdown

Pull request dashboard status

Status last refreshed: 2026-07-23 14:29:53 UTC.

  • Waiting on: Author
  • Next step: Address or respond to 1 review feedback item:
    • Inline threads: 1
    • For each item, reply to move the discussion forward, e.g. link to the commit that addresses it, explain why no change is needed, or ask a follow-up question.

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 /dashboard route:reviewers to route it from waiting on the author to waiting on reviewers. If the last refreshed time above predates your latest reply or push, the dashboard hasn't processed it yet.

…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.
@mvanhorn

Copy link
Copy Markdown
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve metadata for controller instrumentation

3 participants