Skip to content

Fix warnings from the spring boot starter#10086

Merged
laurit merged 3 commits into
open-telemetry:mainfrom
laurit:spring-warning
Dec 22, 2023
Merged

Fix warnings from the spring boot starter#10086
laurit merged 3 commits into
open-telemetry:mainfrom
laurit:spring-warning

Conversation

@laurit

@laurit laurit commented Dec 15, 2023

Copy link
Copy Markdown
Contributor

Resolves #9988
As far as I understand these warnings are printed for beans that are dependencies for one of our BeanPostProcessors. This pr attempts to resolve this be declaring the methods that return a BeanPostProcessor as static to avoid having dependency on the bean that contains the method and instead of OpenTelemetry inject a Supplier to avoid having a direct dependency on OpenTelemetry bean and its dependencies.

@laurit
laurit requested a review from a team December 15, 2023 21:28

@Bean
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
public static OpenTelemetrySupplier openTelemetrySupplier(BeanFactory beanFactory) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please add a comment about the why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

comment added

import java.util.function.Supplier;

/** To inject an OpenTelemetry into bean post processors */
public interface OpenTelemetrySupplier extends Supplier<OpenTelemetry> {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the addition of an interface really needed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the comment specify that the class is for internal use?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The interface name should perhaps mentioned that this interface is only for post processor usage. OpentTelemerySupplierInPostProcessor?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I moved it to internal package, but left the name as is.

Is the addition of an interface really needed?

Do you have a better idea how to do this? To me using a separate interface felt like the simplest approach to ensure that there is only 1 bean that matches.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree. By declaring a bean of type Supplier<OpenTelemetry>, the user could also declare in his code this type of bean and the application will fail to start.

@laurit
laurit merged commit 04da58e into open-telemetry:main Dec 22, 2023
@laurit
laurit deleted the spring-warning branch December 22, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove warning messages from the OTel starter

3 participants