Skip to content

add thread details for declarative config#15209

Open
zeitlinger wants to merge 17 commits intoopen-telemetry:mainfrom
zeitlinger:declarative-config-thread-details-provider2
Open

add thread details for declarative config#15209
zeitlinger wants to merge 17 commits intoopen-telemetry:mainfrom
zeitlinger:declarative-config-thread-details-provider2

Conversation

@zeitlinger
Copy link
Copy Markdown
Member

@zeitlinger zeitlinger commented Nov 3, 2025

Part of #14087
Alternative for #14564

Summary

Moves thread details (thread.id, thread.name) from the AddThreadDetailsSpanProcessor to an InstrumenterCustomizer-based approach using ThreadDetailsAttributesExtractor. This means thread attributes are added via AttributesExtractor at instrumentation build time rather than via SpanProcessor at span start time.

The existing AddThreadDetailsSpanProcessor is still used for non-declarative-config agent mode (where it remains on by default via otel.javaagent.add-thread-details=true). The new InstrumenterCustomizer path is used for declarative config, where it is off by default — users must explicitly enable it:

  • Agent (declarative config): thread_details_enabled: true under the java_agent distribution node (default: false)
  • Spring starter (declarative config): thread_details.enabled: true under spring_starter instrumentation config (default: false)
  • Agent (env var / system properties): otel.javaagent.add-thread-details (default: true, unchanged)

Making it off by default in 3.0 for non-declarative-config users is tracked in #15334.

Approach

  • ThreadDetailsAttributesExtractor in instrumentation-api-incubator captures thread.id and thread.name on span start
  • InstrumenterCustomizerProvider implementations (one for agent, one for spring starter) register the extractor when enabled
  • Agent provider reads from AgentDistributionConfig.isThreadDetailsEnabled()
  • Spring starter provider reads from ExtendedOpenTelemetry.getInstrumentationConfig("spring_starter")
  • Config lives under distribution nodes (not instrumentation.common) per SIG feedback — this is controlled by agent/starter, not individual instrumentations
  • The AddThreadDetailsSpanProcessor registration is removed from AgentTracerProviderConfigurer (the processor class itself is retained for non-DC use)

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for configuring thread details (thread ID and thread name) as span attributes through declarative configuration in the instrumentation API. When enabled via configuration, an AttributesExtractor automatically adds thread information to spans created by instrumenters.

  • Adds ThreadDetailsAttributesExtractor to InstrumenterBuilder to capture thread ID and name
  • Integrates with declarative configuration under instrumentation/java/thread_details/enabled
  • Adds comprehensive test coverage for both enabled and disabled states

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
InstrumenterBuilder.java Implements thread details extraction logic and configuration parsing
AddThreadDetailsTest.java Tests thread details functionality with parameterized enabled/disabled scenarios
build.gradle.kts Adds test dependency for declarative configuration support

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@zeitlinger zeitlinger marked this pull request as draft November 3, 2025 17:55
@zeitlinger zeitlinger force-pushed the declarative-config-thread-details-provider2 branch from 6fb9796 to 29883b5 Compare November 9, 2025 12:51
@zeitlinger zeitlinger marked this pull request as ready for review November 9, 2025 12:51
Copy link
Copy Markdown
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

can you update the PR description with a summary of the relationship between this and the existing thread details span processor? and mention it's off by default when using declarative config (and we can consider making it off by default in 3.0 for others)

@zeitlinger zeitlinger force-pushed the declarative-config-thread-details-provider2 branch from acdc794 to dd2b211 Compare November 17, 2025 12:56
@zeitlinger
Copy link
Copy Markdown
Member Author

and we can consider making it off by default in 3.0 for others)

created #15334

@zeitlinger
Copy link
Copy Markdown
Member Author

@trask please check again

Copy link
Copy Markdown
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

@jaydeluca can you take a look also?

let's also figure out where to document this, ideally in same place as other declarative config "common" attributes

@zeitlinger
Copy link
Copy Markdown
Member Author

@trask please have another look

@zeitlinger zeitlinger force-pushed the declarative-config-thread-details-provider2 branch from fff4d30 to 8c8a96c Compare December 5, 2025 09:06
@zeitlinger zeitlinger added this to the v2.23.0 milestone Dec 5, 2025
@zeitlinger zeitlinger requested a review from trask December 5, 2025 13:07
@trask trask removed this from the v2.23.0 milestone Dec 12, 2025
@zeitlinger
Copy link
Copy Markdown
Member Author

@trask please have another look

@github-actions github-actions Bot added the test native This label can be applied to PRs to trigger them to run native tests label Jan 14, 2026
@zeitlinger zeitlinger force-pushed the declarative-config-thread-details-provider2 branch from 46e9ee9 to aaea68d Compare January 14, 2026 13:59
@zeitlinger
Copy link
Copy Markdown
Member Author

@trask please have a look

Java8RuntimeMetricsProvider and Java17RuntimeMetricsProvider are not
@configuration classes and should not be registered as Spring
auto-configurations. Their presence causes Spring AOT processing to
fail with FileNotFoundException when the classes are not on the
classpath.

Signed-off-by: Gregor Zeitlinger <[email protected]>
The Spring Boot starter's ThreadDetailsInstrumenterCustomizerProvider reads
from the declarative config path spring_starter > thread_details > enabled,
but the ConfigPropertiesBackedDeclarativeConfigProperties bridge only handles
paths starting with "java." or explicitly listed in SPECIAL_MAPPINGS. This
caused the property otel.instrumentation.common.thread-details.enabled to
have no effect in non-declarative-config mode, breaking smoke tests.
Signed-off-by: Gregor Zeitlinger <[email protected]>
…ExtendedOpenTelemetry path

Replace SPI-based ThreadDetailsInstrumenterCustomizerProvider with
ThreadDetailsAutoConfiguration that reads config directly from Spring
Environment, branching on declarative config vs properties-based like
EarlyConfig. Programmatically registers the customizer provider - no
static mutable fields needed.
Signed-off-by: Gregor Zeitlinger <[email protected]>
…ry to fix timing

The previous ThreadDetailsAutoConfiguration ran too late — Spring creates
beans on-demand via DI, so instrumenters were already built before the
flag was set. Moving it into the openTelemetry() bean methods guarantees
correct ordering.
Signed-off-by: Gregor Zeitlinger <[email protected]>
@zeitlinger zeitlinger force-pushed the declarative-config-thread-details-provider2 branch from c975258 to 753691a Compare April 7, 2026 13:24
@zeitlinger
Copy link
Copy Markdown
Member Author

@trask good to merge?

@trask
Copy link
Copy Markdown
Member

trask commented Apr 19, 2026

hey @zeitlinger, the above review is from AI (based on proposal in #17889)

zeitlinger and others added 4 commits April 21, 2026 10:08
…thread-details-provider2

# Conflicts:
#	javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/AgentDistributionConfig.java
- configureDeclarativeConfig now takes ConfigProvider and reads
  java.spring_starter.thread_details.enabled from the instrumentation config.
  The call site in OpenTelemetryAutoConfiguration passes SpringConfigProvider,
  which coerces Spring's flattened String values back to Boolean; the previous
  attempts via SdkConfigProvider/YamlDeclarativeConfigProperties silently dropped
  the flag because Spring's application.yaml pipeline stores every scalar as a
  String and YamlDeclarativeConfigProperties refuses to coerce.
- configureProperties keeps reading otel.instrumentation.common.thread-details.enabled
  from Environment for the properties-based path; no legacy fallback in the
  declarative path.
- Add declarative-config smoke test assertion: enable thread_details in the
  testDeclarativeConfig application.yaml and assert thread.id / thread.name
  appear on the server span.
- Simplify segments[0] == "agent" guard in ConfigPropertiesBackedDeclarativeConfigProperties
  (drop dead segments.length check, work off translatedPath).
@trask
Copy link
Copy Markdown
Member

trask commented Apr 22, 2026

@zeitlinger I just sent zeitlinger#13

Copy link
Copy Markdown
Member Author

I put up an alternative follow-up at zeitlinger#14.

The main reason I didn’t take the config relocation from #13 is that we had already decided in the declarative config SIG to keep this setting under the distro-controlled nodes, not under instrumentation/development/java/.... You called that out here: #15209 (comment)

So #14 keeps the existing spring starter declarative destination otel.distribution.spring_starter.thread_details_enabled, but salvages the useful parts of #13:

  • adds declarative-config smoke coverage that enables the distro-scoped key from application.yaml and asserts thread.id / thread.name
  • takes the unrelated java.agent translation robustness fix

I did not take the ConfigProvider rewrite because, as implemented today, getInstrumentationConfig() only exposes instrumentation/development, not the distro node, so that approach only works together with the relocation.

@trask
Copy link
Copy Markdown
Member

trask commented Apr 30, 2026

keep this setting under the distro-controlled nodes

good point, I'll do a final pass after you merge those updates 👍

Keep declarative thread details distro-scoped
@zeitlinger
Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

trask added 2 commits May 2, 2026 14:55
…thread-details-provider2

# Conflicts:
#	smoke-tests-otel-starter/spring-boot-2/src/testDeclarativeConfig/java/io/opentelemetry/spring/smoketest/OtelSpringStarterSmokeTest.java
…/github.com/trask/opentelemetry-java-instrumentation into declarative-config-thread-details-provider2

# Conflicts:
#	declarative-config-bridge/src/main/java/io/opentelemetry/instrumentation/config/bridge/ConfigPropertiesBackedDeclarativeConfigProperties.java
#	smoke-tests-otel-starter/spring-boot-2/src/testDeclarativeConfig/java/io/opentelemetry/spring/smoketest/OtelSpringStarterSmokeTest.java
@trask
Copy link
Copy Markdown
Member

trask commented May 3, 2026

sorry, more thoughts on this.

I think it's a breaking change and we should hide it behind the v3 preview flag.

and/or maybe we should do both, add the thread attributes in the instrumenter, but also add them as a fallback in the span processor in order to support manual instrumentation done outside of the instrumenter

@github-actions github-actions Bot mentioned this pull request May 5, 2026
@zeitlinger
Copy link
Copy Markdown
Member Author

I think it's a breaking change and we should hide it behind the v3 preview flag.

It should not be breaking - but maybe I miss something.

Or do you mean to support #16459 - which is releated?

@trask
Copy link
Copy Markdown
Member

trask commented May 6, 2026

It should not be breaking - but maybe I miss something.

I was thinking this could break Java agent users who currently get thread.id and thread.name on manually-created spans?

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

Labels

test native This label can be applied to PRs to trigger them to run native tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants