Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ability to disable the automatic Logback appender addition #10629

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Ability to disable the automatic Logback appender addition #10629

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion ...tion/spring/autoconfigure/instrumentation/logging/LogbackAppenderApplicationListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,25 @@ public void onApplicationEvent(ApplicationEvent event) {
if (existingOpenTelemetryAppender.isPresent()) {
reInitializeOpenTelemetryAppender(
existingOpenTelemetryAppender, applicationEnvironmentPreparedEvent);
} else {
} else if (isLogbackAppenderAddable(applicationEnvironmentPreparedEvent)) {
addOpenTelemetryAppender(applicationEnvironmentPreparedEvent);
}
}
}

private static boolean isLogbackAppenderAddable(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the underlying @ConditionalOnProperty to the configuration?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@zeitlinger zeitlinger Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ApplicationEnvironmentPreparedEvent applicationEnvironmentPreparedEvent) {
Boolean otelSdkDisableProperty =
evaluateBooleanProperty(applicationEnvironmentPreparedEvent, "otel.sdk.disabled");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Boolean logbackInstrumentationEnabledProperty =
evaluateBooleanProperty(
applicationEnvironmentPreparedEvent, "otel.instrumentation.logback-appender.enabled");
return otelSdkDisableProperty == null
|| !otelSdkDisableProperty.booleanValue()
|| logbackInstrumentationEnabledProperty == null
|| logbackInstrumentationEnabledProperty.booleanValue();
}

private static void reInitializeOpenTelemetryAppender(
Optional<OpenTelemetryAppender> existingOpenTelemetryAppender,
ApplicationEnvironmentPreparedEvent applicationEnvironmentPreparedEvent) {
Expand Down
Loading