Skip to content

Refactor Spring starter's instrumentation auto configuration classes#8950

Merged
trask merged 4 commits into
open-telemetry:mainfrom
mateuszrzeszutek:refactor-starter-configuration
Jul 21, 2023
Merged

Refactor Spring starter's instrumentation auto configuration classes#8950
trask merged 4 commits into
open-telemetry:mainfrom
mateuszrzeszutek:refactor-starter-configuration

Conversation

@mateuszrzeszutek

Copy link
Copy Markdown
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

@jeanbisutti jeanbisutti left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

trask commented Jul 20, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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