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.

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

Fix warnings from the spring boot starter #10086

Merged
merged 3 commits into from
Dec 22, 2023

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Dec 15, 2023

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
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
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
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
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
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
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
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
47 checks passed
@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