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? |
| if (Objects.equals( | ||
| "otlp", configProperties.getString("otel.traces.exporter", "otlp")) | ||
| && configProperties.getString("otel.exporter.otlp.traces.endpoint") | ||
| == null) { | ||
| properties.put("otel.traces.exporter", "none"); | ||
| } | ||
| if (Objects.equals( | ||
| "otlp", configProperties.getString("otel.metrics.exporter", "otlp")) | ||
| && configProperties.getString("otel.exporter.otlp.metrics.endpoint") | ||
| == null) { | ||
| properties.put("otel.metrics.exporter", "none"); | ||
| } | ||
| if (Objects.equals( | ||
| "otlp", configProperties.getString("otel.logs.exporter", "otlp")) | ||
| && configProperties.getString("otel.exporter.otlp.logs.endpoint") == null) { | ||
| properties.put("otel.logs.exporter", "none"); | ||
| } |
There was a problem hiding this comment.
[minor] it would be more readable to create a helper method to check this as it takes quite an effort to read and understand it that we check two config options to not be explicitly set and set one of them in the modified config, all of that 3 times with close but slightly different variables for logs, traces and metrics.
| if (this.resource == null) { | ||
| this.resource = Resource.empty(); | ||
| } | ||
| if (this.configProperties == null) { | ||
| this.configProperties = DefaultConfigProperties.createFromMap(Collections.emptyMap()); | ||
| } |
There was a problem hiding this comment.
[minor] in order to avoid any ambiguity and possible concurrent modification by the SDK init (I'm pretty sure it's synchronous but we'd better be safe than sorry), it would be better to set this.resource and this.configProperties before the call to AutoConfiguredOpenTelemetrySdk.builder().
| System.clearProperty("otel.exporter.otlp.endpoint"); | ||
| System.clearProperty("otel.service.name"); | ||
| System.clearProperty("otel.resource.attributes"); |
There was a problem hiding this comment.
[minor] As an alternative you could use JUnit @ClearSystemProperty and @SetSystemProperty as it helps to prevent global state side-effects and allows to preserve existing values (if any), see docs for more details.
|
Thanks @SylvainJuge , I'll improve ASAP |
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. >