Load OTel SDK config from environment variables and system properties.… #1434
Conversation
| AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk = | ||
| AutoConfiguredOpenTelemetrySdk.builder() | ||
| .setServiceClassLoader(getClass().getClassLoader()) | ||
| .addPropertiesSupplier(() -> properties) | ||
| .addPropertiesCustomizer(configProperties -> { | ||
| // Change default of "otel.[traces,metrics,logs].exporter" from "otlp" to "none" |
There was a problem hiding this comment.
[minor] Could you elaborate a bit in a comment on why we need to override the default here ? For example, if I understand correctly when there is no endpoint configured then we assume the extension should silently skip exporting signals.
There was a problem hiding this comment.
Thanks for the review Sylvain, please see updated comment
|
@cyrille-leclerc just a heads up looks like there's a failing test |
|
Thanks @trask , unit test fixed. |
|
cc @kenfinnigan in case you have a chance to review |
|
@SylvainJuge could you by any chance review this PR? |
maven-extension/src/main/java/io/opentelemetry/maven/OpenTelemetrySdkService.java
Outdated
Show resolved
Hide resolved
maven-extension/src/main/java/io/opentelemetry/maven/OpenTelemetrySdkService.java
Outdated
Show resolved
Hide resolved
maven-extension/src/test/java/io/opentelemetry/maven/OpenTelemetrySdkServiceTest.java
Show resolved
Hide resolved
|
Thanks @SylvainJuge , I'll improve ASAP |
maven-extension/src/main/java/io/opentelemetry/maven/OpenTelemetrySdkService.java
Show resolved
Hide resolved
maven-extension/src/main/java/io/opentelemetry/maven/OpenTelemetrySdkService.java
Show resolved
Hide resolved
| return properties; | ||
| } | ||
|
|
||
| enum SignalType { |
There was a problem hiding this comment.
[minor] could be made private, also this might be further simplified to an Arrays.asList("traces","metrics","logs").
| ConfigProperties configProperties) { | ||
|
|
||
| Map<String, String> properties = new HashMap<>(); | ||
| if (configProperties.getString("otel.exporter.otlp.endpoint") == null) { |
There was a problem hiding this comment.
[minor style] given most of the code is within this if statement, then inverting the condition and doing an early return with the debug statement avoids a bit of nesting by doing the "quick path" first.
| .disableShutdownHook() | ||
| .build(); | ||
|
|
||
| this.openTelemetrySdk = autoConfiguredOpenTelemetrySdk.getOpenTelemetrySdk(); | ||
|
|
||
| logger.debug("OpenTelemetry: OpenTelemetrySdkService initialized, resource:{}", resource); | ||
|
|
||
| // TODO should we replace `getBooleanConfig(name)` by `configProperties.getBoolean(name)`? |
There was a problem hiding this comment.
[minor] can we answer this TODO directly into this PR or are there still unknowns there to be answered before being able to do this change ?
| System.setProperty("otel.service.name", "my-maven"); | ||
| try { | ||
| testConfiguration("my-maven"); | ||
| System.setProperty("otel.resource.attributes", "key1=val1,key2=val2"); |
There was a problem hiding this comment.
I think it would be a bit better to not rely on global state (here in system properties) as we have to take care of the cleanup here.
What I think could be slightly better is to add an argument to the OpenTelemetrySdkService constructor to control which system properties are present or not if we can. While this slightly alters the design for test purpose it could be worth it here as it allows to get rid of any early cleanup or finally block. Also with such a change there can't be any side effect if system properties are set on the JVM that execute the tests which usually require us to ensure properties are not set in some cases.
The AutoConfiguredOpenTelemetrySdkBuilder.setConfigPropertiesCustomizer allows to override the config properties, but it's package-private so using AutoConfigureUtil.setConfigPropertiesCustomizer is needed. While it's an internal API doing this only for tests should be fine.
Description:
Load OTel SDK config from environment variables and system properties.
Existing Issue(s):
Testing:
See added unit tests
Documentation:
< Describe the documentation added.
Please be sure to modify relevant existing documentation if needed. >
Outstanding items:
< Anything that these changes are intentionally missing
that will be needed or can be helpful in the future. >