-
Notifications
You must be signed in to change notification settings - Fork 845
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
How to break dependency of logger appenders on the global OpenTelemetry instance #8760
Comments
Interesting Spring Boot stack in #8773 |
I don't think that we have much choice in that matter, really; we probably have to implement it the way you described. Logging systems are usually one of the first things to get initialized in an application (and the Spring starter bug is a great example of that), and I can't really think of a better way to handle this.
I think we could do this as an option - users could enable the caching if they decide they really need these startup logs. |
For Logback, the private static Optional<OpenTelemetryAppender> findOtelAppenderWithLogback(LoggerContext loggerContext) {
for (Logger logger : loggerContext.getLoggerList()) {
Iterator<Appender<ILoggingEvent>> appenderIterator = logger.iteratorForAppenders();
while (appenderIterator.hasNext()) {
Appender<ILoggingEvent> appender = appenderIterator.next();
if (appender instanceof OpenTelemetryAppender) {
return Optional.of((OpenTelemetryAppender) appender);
}
}
}
return Optional.empty();
} |
To inject after an |
For Log4j 2, the appenders may be retrieved with this kind of code (not yet tested): List<Appender> appenders = new ArrayList<>();
LoggerContext context = (LoggerContext) LogManager.getContext();
Configuration configuration = context.getConfiguration();
Collection<LoggerConfig> loggerConfigs = configuration.getLoggers().values();
for (LoggerConfig loggerConfig : loggerConfigs) {
Collection<Appender> otherAppenders = loggerConfig.getAppenders().values();
appenders.addAll(otherAppenders);
} |
I'd be in favor of caching some reasonable number of logs by default (and including a warning if there were more, with pointer to how to override the default) |
This may be a heretical question, but why do you consider injecting the SDK into the appender at all? |
This issue is fixed as far as I know. |
@jeanbisutti: Would you mind sharing the details how this was fixed or a link that points to the fix, please? |
@FWinkler79 The updated documentation: https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/logback/logback-appender-1.0/library In your example, you use Spring Boot. With the OTel starter, you don't have to inject an "why not creating an SDK instance in the appender itself ": the OTel starter provides an |
Proposal from last Thu Java SIG meeting is to configure the appenders like normal (e.g. in logback or log4j configuration files), but don't have them use the GlobalOpenTelemetry, and instead have them be noop until someone injects an OpenTelemetry instance into them.
Then in the spring boot starter we can (somehow) dynamically loop through all the appenders (for logback/log4j respectively) and if we find one of our OpenTelemetry Appenders, then we can inject the OpenTelemetry instance.
The limitation here is that we will miss any spring boot startup logs. One option here would be to cache some number of logs in the OpenTelemetry Appenders until the OpenTelemetry instance is injected.
The text was updated successfully, but these errors were encountered: