Skip to content
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

Add metrics & micrometer support to spring-boot-autoconfigure #6270

Merged

Conversation

mateuszrzeszutek
Copy link
Member

Our Spring Boot starter completely did not support metrics, so I decided to add them.

@mateuszrzeszutek mateuszrzeszutek requested a review from a team July 6, 2022 08:57
Comment on lines 17 to 20
implementation("io.opentelemetry:opentelemetry-micrometer1-shim") {
// just get the instrumentation, without the micrometer itself
exclude("io.micrometer", "micrometer-core")
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops 🙈

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

import org.springframework.boot.context.properties.ConfigurationProperties;

/** Configuration for {@link io.opentelemetry.exporter.logging.LoggingSpanExporter}. */
@ConfigurationProperties(prefix = "otel.exporter.logging")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we open sdk issue to add these as sdk autoconfigure properties?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really think so - the spring starter uses a bit different configuration scheme than the SDK (which I dislike, I think they should use exactly the same property names), and the exporters are enabled/disabled in a bit different way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully support changing the sprint boot configuration to match sdk configuration if that's possible

Comment on lines +62 to +67
void noProperties() {
runner.run(
context ->
assertThat(context.getBean("otelLoggingMetricExporter", LoggingMetricExporter.class))
.isNotNull());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do u think this behavior is expected, that the logging exporter is enabled by default?

even if it requires adding the logging exporter as a dependency, I could see that being something handy to have on the classpath for debugging but still not enabled by default

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think that there's definitely something broken in all exporter configurations, between the "enabled by default" and @ConditionalOnMissingBean annotations placed on every other bean. I'd like to tackle it across all exporters in another PR though (and maybe do #923 along the way)

Mateusz Rzeszutek and others added 2 commits July 7, 2022 11:53
@trask trask merged commit 9058ad6 into open-telemetry:main Jul 7, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the spring-boot-autoconfigure-metrics branch July 7, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants