-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load OTel SDK config from environment variables and system properties.… #1434
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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. >