use DeclarativeConfigUtil for library config usage#15656
use DeclarativeConfigUtil for library config usage#15656trask merged 40 commits intoopen-telemetry:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors library configuration access to use DeclarativeConfigUtil instead of directly using ConfigPropertiesUtil, enabling support for declarative configuration. The changes include introducing new utility classes for declarative config support, renaming the otel.experimental.javascript-snippet property to otel.instrumentation.servlet.experimental.javascript-snippet for naming convention consistency, and updating multiple instrumentations (servlet, log4j, kafka, jdbc, aws-sdk) to use the new configuration approach.
Key Changes
- Introduces
DeclarativeConfigUtilandSystemPropertiesBackedDeclarativeConfigPropertiesto enable declarative config support with fallback to system properties - Renames JavaScript snippet property from
otel.experimental.javascript-snippettootel.instrumentation.servlet.experimental.javascript-snippetwith backward compatibility - Refactors AWS SDK v2.2 configuration by consolidating factory logic into
AwsSdkTelemetryFactoryand removing the abstract base class
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/config/internal/DeclarativeConfigUtil.java | New utility class providing main API for accessing declarative config with fallback support |
| instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/config/internal/SystemPropertiesBackedDeclarativeConfigProperties.java | New implementation of DeclarativeConfigProperties backed by system properties and environment variables |
| instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/ConfigUtil.java | New low-level config utilities copied from SDK for safe property access |
| instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/ConfigPropertiesUtil.java | Refactored to use ConfigUtil and added deprecation notices recommending DeclarativeConfig |
| instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java | Added declarative config support check and moved span suppression strategy resolution to instance method |
| instrumentation/servlet/servlet-common/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/servlet/ExperimentalSnippetHolder.java | Updated to use DeclarativeConfigUtil and added deprecation warning for old property name |
| instrumentation/log4j/log4j-context-data/log4j-context-data-2.17/library-autoconfigure/src/main/java/io/opentelemetry/instrumentation/log4j/contextdata/v2_17/internal/ContextDataKeys.java | Migrated to DeclarativeConfigUtil for logging configuration keys |
| instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/src/main/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/TracingProducerInterceptor.java | Migrated to DeclarativeConfigUtil for capture headers configuration |
| instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/src/main/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/TracingConsumerInterceptor.java | Migrated to DeclarativeConfigUtil for receive telemetry and capture headers configuration |
| instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcInstrumenterFactory.java | Updated to use DeclarativeConfigUtil and accept OpenTelemetry parameter for config access |
| instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java | Migrated SQL commenter configuration to DeclarativeConfigUtil with fallback logic |
| instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/AwsSdkTelemetryFactory.java | New factory class consolidating AWS SDK v2.2 telemetry configuration using DeclarativeConfigUtil |
| instrumentation/aws-sdk/aws-sdk-2.2/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/AbstractAwsSdkTelemetryFactory.java | Removed abstract factory class, replaced by concrete AwsSdkTelemetryFactory |
| instrumentation/aws-sdk/aws-sdk-2.2/library-autoconfigure/src/main/java/io/opentelemetry/instrumentation/awssdk/v2_2/autoconfigure/AwsSdkSingletons.java | Simplified to use new AwsSdkTelemetryFactory |
| instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkSingletons.java | Simplified to use new AwsSdkTelemetryFactory |
| instrumentation/aws-sdk/aws-sdk-1.11/library-autoconfigure/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/autoconfigure/TracingRequestHandler.java | Migrated to DeclarativeConfigUtil for AWS SDK v1.11 configuration |
| instrumentation-api-incubator/src/test/java/io/opentelemetry/instrumentation/api/incubator/config/internal/SystemPropertiesBackedDeclarativeConfigPropertiesTest.java | New comprehensive test coverage for SystemPropertiesBackedDeclarativeConfigProperties |
| instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/ConfigPropertiesUtilTest.java | Updated tests to use correct otel.instrumentation prefix |
| instrumentation-api-incubator/build.gradle.kts | Added junit-pioneer dependency and JVM args for Java 17 compatibility |
| instrumentation/servlet/servlet-common/bootstrap/build.gradle.kts | Added compileOnly dependency on instrumentation-api-incubator |
| docs/advanced-configuration-options.md | Updated documentation with new JavaScript snippet property name |
| CHANGELOG.md | Added breaking change entry for JavaScript snippet property rename |
| docs/apidiffs/current_vs_latest/opentelemetry-instrumentation-api.txt | Documents new public isDeclarativeConfig method in InstrumenterBuilder |
| smoke-tests/src/main/java/io/opentelemetry/smoketest/AbstractTestContainerManager.java | Updated smoke tests to use new JavaScript snippet property name |
.../java/io/opentelemetry/instrumentation/awssdk/v1_11/autoconfigure/TracingRequestHandler.java
Outdated
Show resolved
Hide resolved
...ntation/api/incubator/config/internal/SystemPropertiesBackedDeclarativeConfigProperties.java
Outdated
Show resolved
Hide resolved
ba28a2d to
5995f89
Compare
0abf936 to
9bbac6f
Compare
...n/java/io/opentelemetry/instrumentation/api/incubator/config/internal/LibraryConfigUtil.java
Outdated
Show resolved
Hide resolved
...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java
Outdated
Show resolved
Hide resolved
instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/ConfigUtil.java
Outdated
Show resolved
Hide resolved
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Outdated
Show resolved
Hide resolved
90ffe39 to
ef0cae6
Compare
...main/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/TracingConsumerInterceptor.java
Outdated
Show resolved
Hide resolved
smoke-tests/src/main/java/io/opentelemetry/smoketest/AbstractTestContainerManager.java
Outdated
Show resolved
Hide resolved
...ap/src/main/java/io/opentelemetry/javaagent/bootstrap/servlet/ExperimentalSnippetHolder.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/instrumentation/log4j/contextdata/v2_17/internal/ContextDataKeys.java
Outdated
Show resolved
Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcInstrumenterFactory.java
Outdated
Show resolved
Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcInstrumenterFactory.java
Outdated
Show resolved
Hide resolved
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/AwsSdkTelemetryFactory.java
Show resolved
Hide resolved
.../java/io/opentelemetry/instrumentation/awssdk/v1_11/autoconfigure/TracingRequestHandler.java
Show resolved
Hide resolved
.../java/io/opentelemetry/instrumentation/awssdk/v1_11/autoconfigure/TracingRequestHandler.java
Outdated
Show resolved
Hide resolved
2391693 to
90865dd
Compare
| DeclarativeConfigProperties instrumentationConfig = | ||
| ((ExtendedOpenTelemetry) openTelemetry).getConfigProvider().getInstrumentationConfig(); | ||
| if (instrumentationConfig == null) { | ||
| return null; |
There was a problem hiding this comment.
Once getConfigProvider() is moved out of ExtendedOpenTelemetry to OpenTelemetry, I think we'll need to check the sys prop here? If so, may be good to do that now, could be easy to forget when refactoring later
There was a problem hiding this comment.
I added a mapping to check for the old value
.../java/io/opentelemetry/instrumentation/awssdk/v1_11/autoconfigure/TracingRequestHandler.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/AwsSdkTelemetryFactory.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/instrumentation/awssdk/v2_2/internal/AwsSdkTelemetryFactory.java
Outdated
Show resolved
Hide resolved
...on/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/OpenTelemetryDriver.java
Outdated
Show resolved
Hide resolved
...on-api/src/main/java/io/opentelemetry/instrumentation/api/internal/ConfigPropertiesUtil.java
Show resolved
Hide resolved
| // use ConfigPropertiesUtil only to prevent that the deprecated property is used in | ||
| // declarative config |
There was a problem hiding this comment.
i'm not sure what this comment is trying to say
There was a problem hiding this comment.
realized that I can just use SPECIAL_MAPPINGS instead
| boolean enabled = | ||
| DeclarativeConfigUtil.getInstrumentationConfig(openTelemetry, "jdbc") | ||
| .get("sqlcommenter/development") | ||
| .getBoolean("enabled", false); | ||
| if (!enabled) { | ||
| enabled = | ||
| ConfigPropertiesUtil.getBoolean( | ||
| "otel.instrumentation.jdbc.experimental.sqlcommenter.enabled", false); | ||
| } | ||
| if (!enabled) { | ||
| enabled = | ||
| DeclarativeConfigUtil.getInstrumentationConfig(openTelemetry, "common") | ||
| .get("database") | ||
| .get("sqlcommenter/development") | ||
| .getBoolean("enabled", false); | ||
| } | ||
| if (!enabled) { | ||
| enabled = | ||
| ConfigPropertiesUtil.getBoolean( | ||
| "otel.instrumentation.common.experimental.db-sqlcommenter.enabled", false); | ||
| } |
There was a problem hiding this comment.
I think the fallback shouldn't happen if there's an explicit false
I think you need to use the @nullable Boolean returning version and check for null return
| return AwsSdkTelemetry.builder(openTelemetry) | ||
| .setCapturedHeaders( | ||
| messaging.getScalarList( | ||
| "capture_headers/development", | ||
| String.class, | ||
| ConfigPropertiesUtil.getList( | ||
| "otel.instrumentation.messaging.experimental.capture-headers", emptyList()))) |
There was a problem hiding this comment.
thinking more about this, mayb we should keep the two subclasses in order to avoid exposing the javaagent instrumentation to direct system property access
There was a problem hiding this comment.
right - I found a solution without subclassing
39f7544 to
b436af2
Compare
Fixes #15566
Replaces #15339
Part of #15599
Note: This PR takes into account open-telemetry/opentelemetry-java#7927 for method naming.