Resolve ParameterNameDiscoverer Bean Conflict in spring-boot-autoconfigure

[moznion]

This update addresses a conflict arising in the spring-boot-autoconfigure project when using opentelemetry-spring-boot-starter and other projects which use the ParameterNameDiscoverer bean simultaneously, e.g. springdoc-openapi-starter-webmvc-ui.
The issue occurs due to the presence of multiple candidates for the ParameterNameDiscoverer object, leading to an injection conflict.

When both starters are used together, the application fails to start, displaying an error related to the ParameterNameDiscoverer object. This is due to the ambiguity in selecting the correct bean to inject, as multiple candidates are present.

Example of the error output:

***************************
APPLICATION FAILED TO START
***************************

Description:

Parameter 1 of method otelInstrumentationWithSpanAspect in io.opentelemetry.instrumentation.spring.autoconfigure.instrumentation.annotations.InstrumentationAnnotationsAutoConfiguration required a single bean, but 2 were found:
        - parameterNameDiscoverer: defined by method 'parameterNameDiscoverer' in class path resource [io/opentelemetry/instrumentation/spring/autoconfigure/instrumentation/annotations/InstrumentationAnnotationsAutoConfiguration.class]
        - localSpringDocParameterNameDiscoverer: defined by method 'localSpringDocParameterNameDiscoverer' in class path resource [org/springdoc/core/configuration/SpringDocConfiguration.class]

Action:

Consider marking one of the beans as @Primary, updating the consumer to accept multiple beans, or using @Qualifier to identify the bean that should be consumed

To resolve this issue, the @Qualifier annotation has been added to the ParameterNameDiscoverer bean within the spring-boot-autoconfigure project. This annotation assigns a unique label to the ParameterNameDiscoverer bean, ensuring that the project-specific bean is consistently chosen for injection, thereby eliminating the conflict.

Update:
To resolve this issue, this commit defines the private static ParameterNameDiscoverer instance and use that directly instead of injection in InstrumentationAnnotationsAutoConfiguration.

[laurit]

I'm not sure that adding the @Qualifier makes sense here. Looking at

  @Bean
  @ConditionalOnMissingBean
  ParameterNameDiscoverer parameterNameDiscoverer() {
    return new DefaultParameterNameDiscoverer();
  }

my guess would be that the original intent was that user can override this bean by creating their own ParameterNameDiscoverer which would be used in place of this one. Will this still work after adding the @Qualifier? If it does not then might as well not create ParameterNameDiscoverer as a bean but pass the value directly to InstrumentationWithSpanAspect/SdkExtensionWithSpanAspect.

[moznion]

Thank you for your feedback.

my guess would be that the original intent was that user can override this bean by creating their own ParameterNameDiscoverer which would be used in place of this one. Will this still work after adding the @qualifier?

I understand the intention of this design, I didn't realized that. Actually, it wouldn't work as expected if the @Qualifier annotation has been put on the injecter because the custom injecter on the user side becomes to have to have @Qualifier annotation as well.

I guess the root cause of this issue is InstrumentationAnnotationsAutoConfiguration#otelInstrumentationWithSpanAspect detects multiple injection candidates because it uses plain ParameterNameDiscoverer interface directly.

If it does not then might as well not create ParameterNameDiscoverer as a bean but pass the value directly to InstrumentationWithSpanAspect/SdkExtensionWithSpanAspect.

but this either breaks the backward compatibility, right?

Accordingly, I suppose there are three ways to address on this issue.

1. Don't make change

We will attempt to resolve the issue by making adjustments on the client side, without incorporating the changes included like in this pull request.
For example, adding a class like the following to the client's Spring App allows users to force their defined Bean as @Primary, even if there's a conflict with ParameterNameDiscoverer, and can solve the problem in some cases.

@Configuration
public class OpenTelemetryConfig {
    @Bean
    @Primary
    ParameterNameDiscoverer parameterNameDiscoverer() {
        return new DefaultParameterNameDiscoverer();
    }
}

The advantage of this method is that it completely maintains backward compatibility. However, a potential issue is that if there are other classes requiring the ParameterNameDiscoverer instance through DI, the bean with @Primary attached will be injected into all Injectees, which could compromise customizability.

2. Make a change like in this case to add the @Qualifier annotation.

This pull request includes the changes and additionally exposes the qualifier value (i.e. InstrumentationAnnotationsAutoConfiguration.parameterNameDiscoverBeanName). With this change, if the client side is customizing the injector for ParameterNameDiscoverer, it is necessary to add the @Qualifier annotation to that injector as well.

@Configuration
public class OpenTelemetryConfig {
    @Bean
    @Qualifier(InstrumentationAnnotationsAutoConfiguration.parameterNameDiscoverBeanName)
    ParameterNameDiscoverer parameterNameDiscoverer() {
        return new DefaultParameterNameDiscoverer();
    }
}

The pros of this method is that it allows for individual customization even in cases where there are multiple classes requiring ParameterNameDiscoverer via DI, as mentioned in 1). However, this method has the problem of compromising backward compatibility. For example, even if a user has customized the ParameterNameDiscoverer, the introduction of this change on the Instrumentation side means that InstrumentationAnnotationsAutoConfiguration.parameterNameDiscoverer() will be used as the default injector, resulting in the user's custom injector being implicitly ignored.

3. Create a Custom Interface Extending ParameterNameDiscoverer

Create a custom interface that extends the ParameterNameDiscoverer interface and use it within the instrumentation.
Example:

public interface OpenTelemetryInstrumentationParameterNameDiscoverer extends ParameterNameDiscoverer {
}

This approach, which involves using a custom interface instead of applying the @Qualifier annotation, maintains the same advantages and disadvantages mentioned in point 2).

---

What do you think about it? In my mind, 1) should be the simplest one because it doesn't need to intake any changes.

[laurit]

but this either breaks the backward compatibility, right?

so will adding the @Qualifier and changing the type of the ParameterNameDiscoverer bean. I don't like option 1. much because everybody who runs into this issue has to do this. I'd be fine with removing the parameterNameDiscoverer bean and using the DefaultParameterNameDiscoverer directly. I don't think that overriding the ParameterNameDiscoverer is a common use case. @trask @jeanbisutti what do you think?

[jeanbisutti]

For my understanding today, ParameterNameDiscoverer is more an implementation detail for the OpenTelemetry Spring autoconfiguration.

Allowing the injection of a custom ParameterNameDiscoverer (for example with SpringDocParameterNameDiscoverer) into OpenTelemetry code could potentially break the WithSpan OpenTelemetry features.

For some Spring versions, DefaultParameterNameDiscoverer can be based on a LocalVariableTableParameterNameDiscoverer having a cache.

So, I would prefer something like this (less code and complexity than with @Qualifier):

public class InstrumentationAnnotationsAutoConfiguration {

  private final ParameterNameDiscoverer parameterNameDiscoverer = new DefaultParameterNameDiscoverer();

[moznion]

@laurit @jeanbisutti, I appreciate your suggestions. From my understanding, it is uncommon (or unexpected) for users to override the implemented instance of ParameterNameDiscoverer. Therefore, would it be acceptable to create a private final instance of DefaultParameterNameDiscoverer within InstrumentationAnnotationsAutoConfiguration and use it internally instead? Is this understanding correct?

According to my understanding, I made that change at a139db8, what do you think about that?