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

Make spring boot service name detector handle BOOT-INF/classes #8101

Merged

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Mar 21, 2023

When spring boot application is packaged in one jar application.properties and application.yml are under BOOT-INF/classes/.

@laurit laurit requested review from a team and breedx-splk March 21, 2023 11:37
@laurit laurit mentioned this pull request Mar 21, 2023
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Comment on lines +29 to +30
SpringBootServiceNameDetector guesser = new SpringBootServiceNameDetector();
Resource result = guesser.createResource(config);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SpringBootServiceNameDetector guesser = new SpringBootServiceNameDetector();
Resource result = guesser.createResource(config);
SpringBootServiceNameDetector detector = new SpringBootServiceNameDetector();
Resource result = detector.createResource(config);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copy pasted from

SpringBootServiceNameDetector guesser = new SpringBootServiceNameDetector(system);
Resource result = guesser.createResource(config);

WDYT about renaming this in both tests after this is merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sure. I think guesser was in the original name probably. No biggie.

private final boolean addBootInfPrefix;

SystemHelper() {
ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
Copy link
Member

Choose a reason for hiding this comment

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

Is getting the context CL necessary? This class runs in SDK setup time; in the javaagent it shouldn't have any context CL set (I've removed the setContextClassLoader calls a while ago)

Copy link
Contributor Author

@laurit laurit Mar 22, 2023

Choose a reason for hiding this comment

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

There isn't anything that says this has to be used only with javaagent. I think the context class loader defaults to system when it is not set (not set to null or some other class loader by the user).

@mateuszrzeszutek mateuszrzeszutek merged commit d24d798 into open-telemetry:main Mar 28, 2023
@laurit laurit deleted the spring-boot-service-name-detector branch July 6, 2023 17:48
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.

4 participants