-
Notifications
You must be signed in to change notification settings - Fork 41.4k
Add auto-configuration for OpenTelemetry SdkMeterProvider #46640
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Thomas Vitale <[email protected]>
ObjectProvider<SdkMeterProviderBuilderCustomizer> customizers) { | ||
SdkMeterProviderBuilder builder = SdkMeterProvider.builder().setClock(clock).setResource(resource); | ||
if (properties.getExemplars().isEnabled()) { | ||
SdkMeterProviderUtil.setExemplarFilter(builder, exemplarFilter); |
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.
Exemplars are disabled by default because even though the Spec/API is GA, the method in the Java SDK to configure them is not exposed publicly, but through this util. Is that acceptable for Spring Boot?
The API (mapped closely by the configuration properties) won't change as it's stable. So, any change to the SDK internals will not require any code/configuration change from Spring Boot users (a.k.a. stable feature). See: https://opentelemetry.io/docs/specs/otel/metrics/sdk/#exemplar (stable API). More context: open-telemetry/opentelemetry-java#7503.
My recommendation would be to keep this functionality, but disabled by default. Considering that the OTLP Meter Registry doesn't support exemplars yet, this would be the only way in Spring Boot to support exemplars in OpenTelemetry.
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.
That's fine for me.
|
||
@Bean | ||
@ConditionalOnMissingBean | ||
@ConditionalOnBean({ Clock.class, Resource.class }) |
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.
This won't work reliably without auto-configuration ordering, e.g.
@AutoConfiguration(after = OpenTelemetrySdkAutoConfiguration.class)
The logging auto-configuration doesn't have those conditionals, so i think it's okay to drop them here too.
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.
Ok, I'll remove all those conditionals
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.
Removed
ObjectProvider<SdkMeterProviderBuilderCustomizer> customizers) { | ||
SdkMeterProviderBuilder builder = SdkMeterProvider.builder().setClock(clock).setResource(resource); | ||
if (properties.getExemplars().isEnabled()) { | ||
SdkMeterProviderUtil.setExemplarFilter(builder, exemplarFilter); |
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.
That's fine for me.
|
||
@Bean | ||
@ConditionalOnMissingBean | ||
@ConditionalOnBean(OpenTelemetry.class) |
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.
Same ordering issue.
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.
Removed
/** | ||
* Maximum number of distinct points per metric. | ||
*/ | ||
private Integer cardinalityLimit = 2000; |
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.
This could be an int
, couldn't it?
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.
That's true. I'll change it.
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.
Done
* | ||
* @author Thomas Vitale | ||
*/ | ||
class OpenTelemetryMetricsPropertiesTests { |
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.
I don't see much value in these tests, as they only exercise a POJO class.
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.
I'll remove them
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.
Done
Thanks for the PR @ThomasVitale . Before we merge this, we need to decide on #36546 (comment), because this PR provides OTel beans but no way of exporting the metrics to a backend. |
Signed-off-by: Thomas Vitale <[email protected]>
@mhalbritter thanks so much for the review. I have addressed your comments and pushed a second commit (after the final review, I can squash them to provide a single commit). |
metrics
sub-package in thespring-boot-opentelemetry
module.SdkMeterProvider
with customisation options via configuration properties and customizer bean.Clock
bean used to initialise observability signals providers.Fixes gh-36546