Skip to content

Commit 94a0b5e

Browse files
zeitlingerzeitlinger
committed
Address remaining thread-details review comments
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
1 parent 753691a commit 94a0b5e

10 files changed

Lines changed: 98 additions & 40 deletions

File tree

‎declarative-config-bridge/src/main/java/io/opentelemetry/instrumentation/config/bridge/ConfigPropertiesBackedDeclarativeConfigProperties.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,10 @@ private String resolvePropertyKey(String name) {
227227

228228
// java.agent.* maps to otel.javaagent.* (not otel.instrumentation.agent.*)
229229
if (segments.length > 0 && segments[0].equals("agent")) {
230-
return "otel.javaagent." + translatedPath.toString().substring("agent.".length());
230+
if (translatedPath.length() == "agent".length()) {
231+
return "otel.javaagent";
232+
}
233+
return "otel.javaagent." + translatedPath.substring("agent.".length());
231234
}
232235

233236
return "otel.instrumentation." + translatedPath;

‎declarative-config-bridge/src/test/java/io/opentelemetry/instrumentation/config/bridge/ConfigPropertiesBackedDeclarativeConfigPropertiesTest.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,13 @@ void testAgentPrefix() {
8585
.isTrue();
8686
}
8787

88+
@Test
89+
void testAgentPrefixWithoutChild() {
90+
DeclarativeConfigProperties config = createConfig("otel.javaagent.experimental.indy", "true");
91+
92+
assertThat(config.getStructured("java").getString("agent")).isNull();
93+
}
94+
8895
@Test
8996
void testJmxPrefix() {
9097
DeclarativeConfigProperties config = createConfig("otel.jmx.enabled", "true");

‎instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/instrumenter/InstrumenterCustomizer.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55

66
package io.opentelemetry.instrumentation.api.incubator.instrumenter;
77

8-
import io.opentelemetry.api.OpenTelemetry;
98
import io.opentelemetry.context.Context;
109
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
1110
import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
@@ -35,13 +34,6 @@ public interface InstrumenterCustomizer {
3534
*/
3635
String getInstrumentationName();
3736

38-
/**
39-
* Returns the OpenTelemetry instance associated with this instrumenter.
40-
*
41-
* @return the OpenTelemetry instance
42-
*/
43-
OpenTelemetry getOpenTelemetry();
44-
4537
/**
4638
* Tests whether given instrumenter produces telemetry of specified type. Instrumentation type is
4739
* detected based on the standard {@link AttributesExtractor} implementations used by this

‎instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/instrumenter/internal/InstrumenterCustomizerImpl.java‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55

66
package io.opentelemetry.instrumentation.api.incubator.instrumenter.internal;
77

8-
import io.opentelemetry.api.OpenTelemetry;
98
import io.opentelemetry.instrumentation.api.incubator.instrumenter.InstrumenterCustomizer;
109
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
1110
import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
@@ -48,11 +47,6 @@ public String getInstrumentationName() {
4847
return customizer.getInstrumentationName();
4948
}
5049

51-
@Override
52-
public OpenTelemetry getOpenTelemetry() {
53-
return customizer.getOpenTelemetry();
54-
}
55-
5650
@Override
5751
public boolean hasType(InstrumentationType type) {
5852
SpanKey spanKey = typeToSpanKey.get(type);

‎instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -429,11 +429,6 @@ public String getInstrumentationName() {
429429
return builder.instrumentationName;
430430
}
431431

432-
@Override
433-
public OpenTelemetry getOpenTelemetry() {
434-
return builder.openTelemetry;
435-
}
436-
437432
@Override
438433
public boolean hasType(SpanKey type) {
439434
return spanKeys.contains(type);

‎instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InternalInstrumenterCustomizer.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55

66
package io.opentelemetry.instrumentation.api.internal;
77

8-
import io.opentelemetry.api.OpenTelemetry;
98
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
109
import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
1110
import io.opentelemetry.instrumentation.api.instrumenter.OperationMetrics;
@@ -21,8 +20,6 @@ public interface InternalInstrumenterCustomizer<REQUEST, RESPONSE> {
2120

2221
String getInstrumentationName();
2322

24-
OpenTelemetry getOpenTelemetry();
25-
2623
boolean hasType(SpanKey type);
2724

2825
void addAttributesExtractor(AttributesExtractor<REQUEST, RESPONSE> extractor);

‎instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/thread/ThreadDetailsInstrumenterCustomizerProvider.java‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@
1515
* any time.
1616
*/
1717
public class ThreadDetailsInstrumenterCustomizerProvider implements InstrumenterCustomizerProvider {
18+
private static final String LEGACY_PROPERTY =
19+
"otel.instrumentation.common.thread-details.enabled";
20+
private static final String DECLARATIVE_PROPERTY =
21+
"otel.distribution.spring_starter.thread_details_enabled";
1822

1923
// Static because this class is loaded via ServiceLoader (SPI), not managed by Spring DI.
2024
// Set by OpenTelemetryAutoConfiguration#openTelemetry before instrumenters are built,
@@ -24,18 +28,16 @@ public class ThreadDetailsInstrumenterCustomizerProvider implements Instrumenter
2428

2529
/** Called from OpenTelemetryAutoConfiguration for declarative config. */
2630
public static void configureDeclarativeConfig(Environment environment) {
27-
enabled =
28-
Boolean.TRUE.equals(
29-
environment.getProperty(
30-
"otel.distribution.spring_starter.thread_details_enabled", Boolean.class));
31+
// Declarative config uses the distro-scoped key, but we still accept the legacy property so
32+
// both Spring configuration paths resolve the same feature flag.
33+
enabled = readEnabled(environment, DECLARATIVE_PROPERTY, LEGACY_PROPERTY);
3134
}
3235

3336
/** Called from OpenTelemetryAutoConfiguration for properties-based config. */
3437
public static void configureProperties(Environment environment) {
35-
enabled =
36-
Boolean.TRUE.equals(
37-
environment.getProperty(
38-
"otel.instrumentation.common.thread-details.enabled", Boolean.class));
38+
// Properties-based config keeps the legacy toggle, but we fall back to the declarative key so
39+
// a single setting still works if it is shared across config styles.
40+
enabled = readEnabled(environment, LEGACY_PROPERTY, DECLARATIVE_PROPERTY);
3941
}
4042

4143
@Override
@@ -44,4 +46,14 @@ public void customize(InstrumenterCustomizer customizer) {
4446
customizer.addAttributesExtractor(new ThreadDetailsAttributesExtractor<>());
4547
}
4648
}
49+
50+
private static boolean readEnabled(
51+
Environment environment, String primaryProperty, String fallbackProperty) {
52+
Boolean enabled = environment.getProperty(primaryProperty, Boolean.class);
53+
if (enabled != null) {
54+
return enabled;
55+
}
56+
enabled = environment.getProperty(fallbackProperty, Boolean.class);
57+
return Boolean.TRUE.equals(enabled);
58+
}
4759
}

‎instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/thread/ThreadDetailsInstrumenterCustomizerProviderTest.java‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.instrumentation.spring.autoconfigure.internal.instrumentation.thread;
7+
8+
import static org.mockito.ArgumentMatchers.any;
9+
import static org.mockito.Mockito.mock;
10+
import static org.mockito.Mockito.verify;
11+
import static org.mockito.Mockito.verifyNoInteractions;
12+
13+
import io.opentelemetry.instrumentation.api.incubator.instrumenter.InstrumenterCustomizer;
14+
import org.junit.jupiter.api.AfterEach;
15+
import org.junit.jupiter.api.Test;
16+
import org.springframework.mock.env.MockEnvironment;
17+
18+
class ThreadDetailsInstrumenterCustomizerProviderTest {
19+
20+
@AfterEach
21+
void tearDown() {
22+
ThreadDetailsInstrumenterCustomizerProvider.configureProperties(new MockEnvironment());
23+
}
24+
25+
@Test
26+
void configurePropertiesUsesLegacyProperty() {
27+
MockEnvironment environment =
28+
new MockEnvironment().withProperty(
29+
"otel.instrumentation.common.thread-details.enabled", "true");
30+
ThreadDetailsInstrumenterCustomizerProvider.configureProperties(environment);
31+
32+
InstrumenterCustomizer customizer = mock(InstrumenterCustomizer.class);
33+
new ThreadDetailsInstrumenterCustomizerProvider().customize(customizer);
34+
35+
verify(customizer).addAttributesExtractor(any());
36+
}
37+
38+
@Test
39+
void configureDeclarativeConfigUsesDistributionProperty() {
40+
MockEnvironment environment =
41+
new MockEnvironment().withProperty(
42+
"otel.distribution.spring_starter.thread_details_enabled", "true");
43+
ThreadDetailsInstrumenterCustomizerProvider.configureDeclarativeConfig(environment);
44+
45+
InstrumenterCustomizer customizer = mock(InstrumenterCustomizer.class);
46+
new ThreadDetailsInstrumenterCustomizerProvider().customize(customizer);
47+
48+
verify(customizer).addAttributesExtractor(any());
49+
}
50+
51+
@Test
52+
void disabledDoesNotCustomize() {
53+
ThreadDetailsInstrumenterCustomizerProvider.configureProperties(new MockEnvironment());
54+
55+
InstrumenterCustomizer customizer = mock(InstrumenterCustomizer.class);
56+
new ThreadDetailsInstrumenterCustomizerProvider().customize(customizer);
57+
58+
verifyNoInteractions(customizer);
59+
}
60+
}

‎javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/AgentDistributionConfig.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,9 @@ private static final class ConfigPropertiesAgentDistributionConfig
234234
"otel.javaagent.experimental.force-synchronous-agent-listeners", false),
235235
configProperties.getList("otel.javaagent.exclude-classes"),
236236
configProperties.getList("otel.javaagent.exclude-class-loaders"),
237-
configProperties.getBoolean("otel.javaagent.add-thread-details", true),
237+
configProperties.getBoolean(
238+
"otel.javaagent.add-thread-details",
239+
!configProperties.getBoolean("otel.instrumentation.common.v3-preview", false)),
238240
null);
239241
this.configProperties = configProperties;
240242
}

‎javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/config/ThreadDetailsInstrumenterCustomizerProviderTest.java‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package io.opentelemetry.javaagent.tooling.config;
77

88
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.satisfies;
9+
import static io.opentelemetry.api.common.Attributes.empty;
910
import static io.opentelemetry.semconv.incubating.ThreadIncubatingAttributes.THREAD_ID;
1011
import static io.opentelemetry.semconv.incubating.ThreadIncubatingAttributes.THREAD_NAME;
1112
import static java.util.Collections.emptyMap;
@@ -43,21 +44,16 @@ public static Stream<Arguments> allEnabledAndDisabledValues() {
4344
(Consumer<SpanDataAssert>)
4445
span ->
4546
span.hasAttributesSatisfying(
46-
satisfies(THREAD_ID, n -> n.isNotNull()),
47-
satisfies(THREAD_NAME, n -> n.isNotBlank()))),
47+
satisfies(THREAD_ID, n -> n.isEqualTo(Thread.currentThread().getId())),
48+
satisfies(THREAD_NAME, n -> n.isEqualTo(Thread.currentThread().getName())))),
4849
Arguments.of(
4950
false,
50-
(Consumer<SpanDataAssert>)
51-
span ->
52-
span.hasAttributesSatisfying(
53-
satisfies(THREAD_ID, n -> n.isNull()),
54-
satisfies(THREAD_NAME, n -> n.isNull()))));
51+
(Consumer<SpanDataAssert>) span -> span.hasAttributes(empty())));
5552
}
5653

5754
@ParameterizedTest(name = "enabled={0}")
5855
@MethodSource("allEnabledAndDisabledValues")
5956
void enabled(boolean enabled, Consumer<SpanDataAssert> spanAttributesConsumer) {
60-
AgentDistributionConfig.resetForTest();
6157
AgentDistributionConfig.set(
6258
AgentDistributionConfig.fromConfigProperties(
6359
DefaultConfigProperties.createFromMap(

0 commit comments

Comments
 (0)