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

Refactor Spring starter's instrumentation auto configuration classes #8950

Merged

Conversation

mateuszrzeszutek
Copy link
Member

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.

@mateuszrzeszutek mateuszrzeszutek requested a review from a team July 14, 2023 14:06
@github-actions github-actions bot requested a review from theletterf July 14, 2023 14:06
Copy link
Member

@jeanbisutti jeanbisutti left a 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.

@mateuszrzeszutek
Copy link
Member Author

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).

@jeanbisutti
Copy link
Member

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.

@trask
Copy link
Member

trask commented Jul 20, 2023

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

Comment on lines +403 to +406
| 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 |
Copy link
Member

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?

Copy link
Member Author

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

@mateuszrzeszutek
Copy link
Member Author

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

Yeah, we'll definitely want to have them for troubleshooting.

@trask trask enabled auto-merge (squash) July 20, 2023 17:00
@trask trask merged commit fd8e361 into open-telemetry:main Jul 21, 2023
@mateuszrzeszutek mateuszrzeszutek deleted the refactor-starter-configuration branch July 21, 2023 11:42
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.

3 participants