add thread details for declarative config #15209
Conversation
There was a problem hiding this comment.
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
ThreadDetailsAttributesExtractortoInstrumenterBuilderto 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.
...pi/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/AddThreadDetailsTest.java
Show resolved
Hide resolved
...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java
Show resolved
Hide resolved
...pi/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/AddThreadDetailsTest.java
Show resolved
Hide resolved
zeitlinger
force-pushed
the
declarative-config-thread-details-provider2
branch
from
6fb9796 to
29883b5
Compare
| private static boolean isIncubator() { | ||
| try { | ||
| Class.forName("io.opentelemetry.api.incubator.ExtendedOpenTelemetry"); | ||
| return true; | ||
| } catch (ClassNotFoundException e) { | ||
| // incubator module is not available | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
instrumentation api depends on io.opentelemetry:opentelemetry-api-incubator so this check shouldn't evaluate to false
There was a problem hiding this comment.
it does for tests in the otel instrumentation API, e.g.
java.lang.NoClassDefFoundError: io/opentelemetry/api/incubator/ExtendedOpenTelemetry
at io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder.addThreadDetailsAttributeExtractor(InstrumenterBuilder.java:471)
at io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder.buildInstrumenter(InstrumenterBuilder.java:319)
at io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder.buildInstrumenter(InstrumenterBuilder.java:311)
at io.opentelemetry.instrumentation.testing.TestInstrumenters.<init>(TestInstrumenters.java:41)
at io.opentelemetry.instrumentation.testing.InstrumentationTestRunner.<init>(InstrumentationTestRunner.java:68)
at io.opentelemetry.instrumentation.testing.AgentTestRunner.<init>(AgentTestRunner.java:47)
at io.opentelemetry.instrumentation.testing.AgentTestRunner.<clinit>(AgentTestRunner.java:40)
at io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension.<init>(AgentInstrumentationExtension.java:35)
at io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension.create(AgentInstrumentationExtension.java:39)
at io.opentelemetry.javaagent.instrumentation.opentelemetryapi.v1_50.logs.LoggerTest.<clinit>(LoggerTest.java:34)
at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized0(Native Method)
at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized(Unsafe.java:1160)
at java.base/java.lang.reflect.Field.acquireOverrideFieldAccessor(Field.java:1200)
at java.base/java.lang.reflect.Field.getOverrideFieldAccessor(Field.java:1169)
at java.base/java.lang.reflect.Field.get(Field.java:444)
at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395)
at java.base/java.util.stream.DistinctOps$1$2.end(DistinctOps.java:168)
at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:261)
at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:510)
at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.ClassNotFoundException: io.opentelemetry.api.incubator.ExtendedOpenTelemetry
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
... 27 more
There was a problem hiding this comment.
Understood. It would be nice to have a comment that explains why this check is needed. Essentially it is a problem in that test. Or test harness uses instrumenter, but opentelemetry-api tests force using old versions of opentelemetry-api that could be missing features. It is not the first time we have run into issues with these tests. The problem could probably also be solved by delaying initialization of TestInstrumenters, that logger test shouldn't need it, though there could be some other test where this approach doesn't work.
|
|
||
| private static <RESPONSE, REQUEST> void addThreadDetailsAttributeExtractor( | ||
| InstrumenterBuilder<REQUEST, RESPONSE> builder) { | ||
| if (isIncubator && builder.openTelemetry instanceof ExtendedOpenTelemetry) { |
There was a problem hiding this comment.
I'm not sure about this. Currently we attach AddThreadDetailsSpanProcessor only in agent/spring boot and offer a non-declarative config option to disable it. Are you sure this needs to be baked into the instrumentation-api? Perhaps we should maintain the current behavior and only add it for agent/spring starter? I believe we could use the instrumenter cusomizers for this. If we are going to bake it in I suspect we'll have to provide a different way for disabling it since OpenTelemetry instance could be produced without the declarative config.
There was a problem hiding this comment.
we'll have to provide a different way for disabling it
ExtendedOpenTelemetry is only returned for declarative config and thread details are disabled by default
Are you sure this needs to be baked into the instrumentation-api
I guess we have the opportunity to decide how we image configuration going forward.
The property name doesn't include agent or spring, so it should always apply - that's the design we currently have.
There was a problem hiding this comment.
Perhaps we should maintain the current behavior and only add it for agent/spring starter?
I think it's ok for this to be a "common" configuration setting. Whether all instrumentation supports it is a different question, which is similar to our other "common" configuration settings.
...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java
Show resolved
Hide resolved
trask
left a comment
trask
left a comment
There was a problem hiding this comment.
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
force-pushed
the
declarative-config-thread-details-provider2
branch
from
acdc794 to
dd2b211
Compare
created #15334 |
|
@trask please check again |
3 participants
Part of #14087
Alternative for #14564
Notes
overview of different use cases