Initialize appenders in the spring boot starter #8888
Conversation
|
@mateuszrzeszutek You could continue from your PR? I can help in the review for example. |
Sure! That'd be very helpful, thanks. I'll just add more tests to make sure it works, and I think this should be ready for review. |
It would be interesting to have JVM + Spring native tests for this type of use case: OTel starter + logging instrumentation libraries. Related issues: #8773, #8776 |
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnProperty( |
There was a problem hiding this comment.
Why not to enable the OpenTelemetry injection by default? Check on line 44 is not sufficient?
There was a problem hiding this comment.
It is enabled by default; in case the property is absent it still gets evaluated to true. I added it to make it possible to disable the appender auto configuration.
There was a problem hiding this comment.
Sorry, I was not rather explicit. My point is that I don't see the use case for which the user would like to disable the automatic injection of the OpenTelemetry instance into the OTel appender. I would prefer to remove the property if we don't have a use case in mind.
| @@ -141,9 +140,6 @@ public OpenTelemetry openTelemetry( | |||
|
|
|||
| ContextPropagators propagators = propagatorsProvider.getIfAvailable(ContextPropagators::noop); | |||
|
|
|||
| // global is needed for logging appenders | |||
| GlobalOpenTelemetry.set(OpenTelemetrySdk.builder().setLoggerProvider(loggerProvider).build()); | |||
There was a problem hiding this comment.
An OpenTelemtry instance is built twice: on line 145 and just after
...try/instrumentation/spring/autoconfigure/logging/OpenTelemetryAppenderAutoConfiguration.java
Show resolved
Hide resolved
...try/instrumentation/spring/autoconfigure/logging/OpenTelemetryAppenderAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...try/instrumentation/spring/autoconfigure/logging/OpenTelemetryAppenderAutoConfiguration.java
Outdated
Show resolved
Hide resolved
| compileOnly(project(":instrumentation:log4j:log4j-appender-2.17:library")) | ||
| compileOnly("org.apache.logging.log4j:log4j-core:2.17.0") | ||
| compileOnly(project(":instrumentation:logback:logback-appender-1.0:library")) | ||
| compileOnly("ch.qos.logback:logback-classic:1.0.0") |
There was a problem hiding this comment.
what do you think about pulling in the appender instrumentation by default, similar to the other library instrumentations?
There was a problem hiding this comment.
created #8925 to track/discuss further, will merge this PR as is for the upcoming 1.28.0 release
There was a problem hiding this comment.
I tested a similar feature by creating a new module that includes log4j but excludes logback: https://github.com/grafana/grafana-opentelemetry-starter/tree/main/integrationTests/log4j
| ApplicationListener<ApplicationReadyEvent> log4jOtelAppenderInitializer( | ||
| OpenTelemetry openTelemetry) { | ||
| return event -> { | ||
| io.opentelemetry.instrumentation.log4j.appender.v2_17.OpenTelemetryAppender.install( |
There was a problem hiding this comment.
what do you think about adding the otel appender if it doesn't exist? One thing less for the user to do.
Here's how I did it: https://github.com/grafana/grafana-opentelemetry-starter/blob/main/src/main/java/com/grafana/opentelemetry/Log4jConfig.java
There was a problem hiding this comment.
It would be great. We would have to provide a way to customize the OTel automatically added. Today, users can do this with an XML configuration in their Spring Boot projects. It could be done for example by providing a Spring bean of the OTel appender automatically added. The user could then customize this bean with a Spring post-processor.
The automatic addition of the OTel appenders could perhaps be discussed in an issue and not be implemented in this PR?
There was a problem hiding this comment.
We would have to provide a way to customize the OTel automatically added
Users can still add the appender (using XML) if they want to have some special settings.
My proposal is to add the appender only if no otel appender is found.
separate issue
sure, I don't mind 😄
There was a problem hiding this comment.
I have created an issue for tracking and discussions: #8920
|
Considering that @mateuszrzeszutek is unavailable until Thursday can we merge this as is? |
I tried to do that in this PR as well -- but since |
1 similar comment
I tried to do that in this PR as well -- but since |
By coincidence I've started working on the same thing as @jeanbisutti in #8887
Still no tests though, I've been hoping to utilize the gradle test suites for that