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

Spring boot service name #8006

Merged
merged 17 commits into from
Apr 2, 2023

Conversation

adamleantech
Copy link
Contributor

resolves #7998

@laurit
Copy link
Contributor

laurit commented Mar 8, 2023

@adamleantech currently this pr also contains your logback baggage changes

@adamleantech
Copy link
Contributor Author

@adamleantech currently this pr also contains your logback baggage changes

@laurit yeah, i wanted to leave it in draft until rebasing the merge of the baggage changes, should be good now

@adamleantech adamleantech marked this pull request as ready for review March 9, 2023 16:52
@adamleantech adamleantech requested a review from a team March 9, 2023 16:52
@adamleantech adamleantech reopened this Mar 9, 2023
@laurit
Copy link
Contributor

laurit commented Mar 9, 2023

Closing and reopening to trigger checks

@laurit laurit closed this Mar 9, 2023
@laurit laurit reopened this Mar 9, 2023
@Nullable
private String findByBootInfApplicationYml() {
String result =
loadFromBootInf("application.yml", SpringBootServiceNameDetector::parseNameFromYaml);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you've found a bug in the findByClasspath*() methods - they both should in fact be loading files from the BOOT-INF/classes directory. Can you fix these two instead of adding this new method?

Copy link
Contributor

Choose a reason for hiding this comment

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

To me this looks wrong. @adamleantech why do yo need to explicitly mention BOOT-INF/classes, spring boot class loader automatically handles finding stuff from BOOT-INF/classes you shouldn't need to add it to resource path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the java agent executes this code before the spring boot app has had chance to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mateuszrzeszutek I found the existing functionality useful when testing the agent against spring boot apps in my IDE where the code has not been assembled into a JAR and therefore the resources are in the usual classpath resource location. Maybe findByClasspath*() should be updated to look in both places?

Copy link
Contributor

Choose a reason for hiding this comment

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

@adamleantech Thanks, I have added tests and fixed BOOT-INF/classes handling in #8101. Could you review and see if this would work for your use case. If it does you could remove BOOT-INF/classes related code from this PR and only leave handling multiple yaml.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @adamleantech I just merged #8101; can you merge/rebase your PR with main?

@mateuszrzeszutek mateuszrzeszutek added this to the v1.24.0 milestone Mar 13, 2023
@laurit laurit removed this from the v1.24.0 milestone Mar 15, 2023
@trask trask merged commit d6271cc into open-telemetry:main Apr 2, 2023
@trask
Copy link
Member

trask commented Apr 2, 2023

thanks @adamleantech!

@adamleantech adamleantech deleted the spring-boot-service-name branch April 3, 2023 11:50
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.

service.name attribute should be parseable from application.yml when more than one yaml file is defined
4 participants