Ability to disable the automatic Logback appender addition

[jeanbisutti]

No description provided.

[zeitlinger]

why not use @Conditional(SdkEnabled.class) here instead?

[jeanbisutti]

The application listener is executed very early at the application startup. @Conditional(SdkEnabled.class) can't work. The property has to be retrieved from an application event.

[zeitlinger]

Could you add the underlying @ConditionalOnProperty to the configuration?

[jeanbisutti]

why not use @Conditional(SdkEnabled.class) here instead?

I did not read your question correctly . The link you gave is related to application listeners allowing to inject an OpenTelemetry instance into the OpenTelemetry appenders. This PR has a different goal. It aims to be able to disable the automatic addition of the Logback OpenTelemetry appender. You have already added @Conditional(SdkEnabled.class) to the autoconfiguration creating the listeners allowing the injection of the OpenTelemetry instance into the OpenTelemetry appenders in #10602:

opentelemetry-java-instrumentation/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/instrumentation/logging/OpenTelemetryAppenderAutoConfiguration.java

Line 21 in b0bbbd1

@Conditional(SdkEnabled.class)

[zeitlinger]

Right - next try: check for sdk disabled at the start of the method to make it very clear:

public void onApplicationEvent(ApplicationEvent event) {
    if (event instanceof ApplicationEnvironmentPreparedEvent // Event for which
    // org.springframework.boot.context.logging.LoggingApplicationListener
    // initializes logging
    ) {
      ApplicationEnvironmentPreparedEvent applicationEnvironmentPreparedEvent =
          (ApplicationEnvironmentPreparedEvent) event;
      Boolean otelSdkDisableProperty =
          evaluateBooleanProperty(applicationEnvironmentPreparedEvent, "otel.sdk.disabled");
      if (otelSdkDisableProperty != null && otelSdkDisableProperty) {
        return;
      }
      
      Optional<OpenTelemetryAppender> existingOpenTelemetryAppender = findOpenTelemetryAppender();
      if (existingOpenTelemetryAppender.isPresent()) {
        reInitializeOpenTelemetryAppender(
            existingOpenTelemetryAppender, applicationEnvironmentPreparedEvent);
      } else if (isLogbackAppenderAddable(applicationEnvironmentPreparedEvent)) {
        addOpenTelemetryAppender(applicationEnvironmentPreparedEvent);
      }
    }
  }

[jeanbisutti]

Right - next try: check for sdk disabled at the start of the method to make it very clear:

public void onApplicationEvent(ApplicationEvent event) {
    if (event instanceof ApplicationEnvironmentPreparedEvent // Event for which
    // org.springframework.boot.context.logging.LoggingApplicationListener
    // initializes logging
    ) {
      ApplicationEnvironmentPreparedEvent applicationEnvironmentPreparedEvent =
          (ApplicationEnvironmentPreparedEvent) event;
      Boolean otelSdkDisableProperty =
          evaluateBooleanProperty(applicationEnvironmentPreparedEvent, "otel.sdk.disabled");
      if (otelSdkDisableProperty != null && otelSdkDisableProperty) {
        return;
      }
      
      Optional<OpenTelemetryAppender> existingOpenTelemetryAppender = findOpenTelemetryAppender();
      if (existingOpenTelemetryAppender.isPresent()) {
        reInitializeOpenTelemetryAppender(
            existingOpenTelemetryAppender, applicationEnvironmentPreparedEvent);
      } else if (isLogbackAppenderAddable(applicationEnvironmentPreparedEvent)) {
        addOpenTelemetryAppender(applicationEnvironmentPreparedEvent);
      }
    }
  }

It would remove the ability to reconfigure an OpenTelemetry Logback appender defined in an XML file from system properties.

Imagine a user having configured the capture of code attributes from an OTel Logback appender in an XML file. The user has packaged his application and is using it in production. The capture of code attributes is expensive and can cause performance penalty. It would not be possible to disable the capture of log attributes without repacking the application and redeliver it in production.

[laurit]

Perhaps it would make sense to add a variant of evaluateBooleanProperty that returns boolean and accepts the default value as an argument.

[jeanbisutti]

For me it seems a little early to add this method today because (if I understand correctly):

• The method would be called at only two places
• The default value as an argument would be true in both cases

[jeanbisutti]

Can this one be merged?