Avoid unecessary addition of the Logback appender

[jfbreault]

No description provided.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jfbreault (b49d2bb)

[laurit]

@zeitlinger could you take a look

[zeitlinger]

looks like it was working as intended - otel.sdk.disabled=true should disable all instrumentations - it's a global switch that takes priority

[jfbreault]

If the global swith is not provided and you provide otel.instrumentation.logback-appender.enabled=false the appender is currently registered

[jfbreault]

As soon as return otelSdkDisableProperty == null is evaluate to true, it returns. It will never check otel.instrumentation.logback-appender.enabled value

[zeitlinger]

got it - reopening

[zeitlinger]

thanks for the contribution.

please add a test to

opentelemetry-java-instrumentation/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/instrumentation/logging/LogbackAppenderTest.java

Line 97 in 80e5bac

void shouldNotInitializeAppenderWhenDisabled() {

- maybe parameterized

[trask]

hi @jfbreault! are you able to sign the CLA above? #11610 (comment)

[jfbreault]

Cla signed, test added, everything see to work.

Btw, the autoconfiguration seems weird and the shouldNotInitializeAppenderWhenDisabled() is a false positive.

spring.factories will be loaded even if otel.springboot.logback-appender.enabled=false so registration will occur. To make it simple, the otel.springboot.logback-appender.enabled don't do anything probably except disabling trying to register it a second time

[zeitlinger]

Cla signed, test added, everything see to work.

Btw, the autoconfiguration seems weird and the shouldNotInitializeAppenderWhenDisabled() is a false positive.

spring.factories will be loaded even if otel.springboot.logback-appender.enabled=false so registration will occur. To make it simple, the otel.springboot.logback-appender.enabled don't do anything probably except disabling trying to register it a second time

I think this was the best solution we could find. cc @jeanbisutti

[jeanbisutti]

The new tests seem unrelated to the code update.

The modified code is for the automatic addition of the Logback appender if the user has not declared the OpenTelemetry Logback appender in a Logback XML file. The new tests use a Logback XML file.

If the global swith is not provided and you provide otel.instrumentation.logback-appender.enabled=false the appender is currently registered

Even it's the case, the appender won't produce log records See

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

Line 35 in de11929

@ConditionalOnEnabledInstrumentation(module = "logback-appender")

I am off this week. I could have another look the week after.

[jeanbisutti]

@jfbreault If there is a regression, could please provide a test reproducing the regression or explain how to reproduce it?

[jfbreault]

@jeanbisutti It's not a regression, it's somthing I found when debugging my issue, it's should be an other issue/pr if we want to fix it. I just wanted to higlight it

[jeanbisutti]

See #11610 (review)

[jfbreault]

The regression is adding the support of the global flag otel.sdk.disabled. Currently, if you don't provide any value for otel.sdk.disabled and provide otel.instrumentation.logback-appender.enabled=false, it will register the appender because the code will return true as soon as it see otelSdkDisableProperty == null

The other issue that I was highlighting is the autoconfiguration registering an event listener to add the appender is probably unnecessary as the listener is already registered in spring.factories so it try to register it a second time, the otel.springboot.logback-appender.enabled=false do nothing in reality. The current unit test for this case is probably not loading the spring.factories file and that why it's probably passing, hence the false positive i was talking. I may provide a test and a use case to prove it in another pr or issue.

Anyway, with this pr, we can disable the registration with the standard otel flag

[jeanbisutti]

The regression is adding the support of the global flag otel.sdk.disabled. Currently, if you don't provide any value for otel.sdk.disabled and provide otel.instrumentation.logback-appender.enabled=false.

In this case, OpenTelemetry won't be installed into the OpenTelemetry Logback appender, and the appender won't emit log records. So, from a user perspective, the OpenTelemetry Logback appender is disabled. If otel.instrumentation.logback-appender.enabled=false is added to this test class, a test will fail because of the absence of log records. Having said that, it does not seem a regression.

The tests you have added in LogbackAppenderTest.java use properties.put("logging.config", "classpath:logback-test.xml"). This test class is dedicated to the case for which the user adds the OpenTelemetry Logback appender to a Logback XML file. Your code change is not related to this. Could you please remove the two tests that are not related to your code change? I will approve the PR once the two unrelated tests are removed.

I have also suggested two changes about the naming.

If you want to add a test with otel.instrumentation.logback-appender.enabled=false and without a Logback XML file, please use the smoke-tests-otel-starter folder. You can take inspiration from this test class.

PRs and issues are welcome if you think that something is not working as expected or if you want to add more tests.

[piclemx]

Hello @jeanbisutti @zeitlinger and @jfbreault, what's the status on this?

I agree with @jfbreault on the way he did implement it and this is fixing also some of my issue.

Thanks!

[jeanbisutti]

@jfbreault We could merge the PR with the few changes asked above.

[jeanbisutti]

@piclemx
From what I understand, the implementation changes in this PR seem more about code cleaning than a fix of an issue: #11610 (comment)

If you observe an issue, could you please provide the steps to reproduce it? We need to be able to reproduce the problem to be able to fix it.