-
Notifications
You must be signed in to change notification settings - Fork 859
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
Refactor Spring starter's instrumentation auto configuration classes #8950
Refactor Spring starter's instrumentation auto configuration classes #8950
Conversation
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.
Perhaps the following classes could also be removed:
LoggingSpanExporterAutoConfiguration
LoggingMetricExporterAutoConfiguration
LoggingExporterProperties
LoggingSpanExporterAutoConfiguration
and LoggingMetricExporterAutoConfiguration
only display the spans and metrics in the logs.
I think we'll probably want to leave the logging exporters as built-ints, just for debug. Their configurations should definitely be refactored though (in a separate PR). |
"just for debug": I imagine it is for OTel developers? In this case, they could use the IDE debugger or add in-memory logger exporter declared as a Spring bean and logging the log data? My understanding is that users will use the OTLP logger exporter enabled by default. I would prefer to try to limit the features to what the users will use. So, less code to maintain and less configuration to document. |
I think logging exporters are pretty convenient for users who are onboarding to otel instrumentation and haven't set up their ingestion pipeline yet, or for users who are troubleshooting issues and want to eliminate the ingestion pipeline from the equation |
| spring-web | otel.instrumentation.spring-webmvc.enabled | `true` | RestTemplate | | ||
| spring-webmvc | otel.instrumentation.spring-web.enabled | `true` | OncePerRequestFilter | | ||
| spring-webflux | otel.instrumentation.spring-webflux.enabled | `true` | WebClient | | ||
| @WithSpan | otel.instrumentation.annotations.enabled | `true` | WithSpan, Aspect | |
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.
nice
should we add kafka, micrometer, logback-appender, log4j-appender to the table also?
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.
We should rewrite the whole doc 😅
I'm kind of hesitant about adding new things to this one, I know we'll have to modify it significantly before we move it to opentelemetry.io
Yeah, we'll definitely want to have them for troubleshooting. |
Part of #2481
I renamed the configuration properties and tried to make them similar/same as in the javaagent (
otel.instrumentation.<name>.enabled
). I also removed some not really needed classes, and reduced the public API of the starter -- spring auto configuration classes are essentially the public API of this module, since the user needs to be able to do@AutoConfigure(Before|After)
for our own configs.