How to break dependency of logger appenders on the global OpenTelemetry instance

[trask]

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.

[jeanbisutti]

Interesting Spring Boot stack in #8773

[mateuszrzeszutek]

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.

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.

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.

I think we could do this as an option - users could enable the caching if they decide they really need these startup logs.

[jeanbisutti]

For Logback, the OpenTelemetryAppender could be dynamically retrieved with this kind of code:

    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();
    }

[jeanbisutti]

For Logback, the OpenTelemetryAppender could be dynamically retrieved with this kind of code:

    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 OpenTelemetry instance, a set(OpenTelemetry) method would have to be added to the Logback OpenTelemetryAppender.

[jeanbisutti]

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);
      }

[trask]

users could enable the caching if they decide they really need these startup logs

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)

[FWinkler79]

This may be a heretical question, but why do you consider injecting the SDK into the appender at all?
I think Logback and Log4J appenders are not especially injection-friendly, and even though from a dependency-injection pov it may not be the nicest solution, why not creating an SDK instance in the appender itself (just with the required OTLP log exporters) and configuring it with properties in the logback.xml file? In case anyone wants to see what this would look like you can find sample code here.

[jeanbisutti]

This issue is fixed as far as I know.

[FWinkler79]

@jeanbisutti: Would you mind sharing the details how this was fixed or a link that points to the fix, please?
I'd really be curious how this is handled now.
Thanks!

[jeanbisutti]

@FWinkler79 The updated documentation: https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/logback/logback-appender-1.0/library

PRs: #8791, #8888, #9798

In your example, you use Spring Boot. With the OTel starter, you don't have to inject an OpenTelemetry instance into the OTel log appender. It is automatically done by this starter. You have just to add the OTel appender inthe logback.xml or log4j2.xml file. This remaining thing could probably be done automatically by the OTel starter.

"why not creating an SDK instance in the appender itself ": the OTel starter provides an OpenTelemetry bean that can bean injected into other Spring beans.