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 starter: add service.version detection, improve service.name detection #10457

Merged
merged 7 commits into from
Feb 21, 2024

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Feb 9, 2024

Don't use the spring resource detector library, because that inspects a spring application from the outside - inside a spring app it's much easier.

service.name is detected in the following order (highest priority first)

  • otel.service.name
  • otel.resource.attributes entry
  • spring.application.name
  • maven/gradle project name created by snipped below (maven)
  • name of jar file
springBoot {
  buildInfo()
}

Fixes #10368

@zeitlinger zeitlinger self-assigned this Feb 9, 2024
@zeitlinger zeitlinger requested a review from a team as a code owner February 9, 2024 17:23
@zeitlinger zeitlinger force-pushed the spring-resource-detector branch 2 times, most recently from 664d560 to a2c210a Compare February 11, 2024 09:25
@zeitlinger zeitlinger changed the title add service.version detection to spring boot starter spring boot starter: add service.version detection, improve service.name detection Feb 11, 2024
@zeitlinger
Copy link
Member Author

@jeanbisutti in this PR, where the test uses graalvm native, the following is needed to read the version:

springBoot {
  buildInfo()
}

In smoke-tests (no graalvm native), it's not needed: #10515

Does this make sense?

@jeanbisutti
Copy link
Member

@jeanbisutti in this PR, where the test uses graalvm native, the following is needed to read the version:

springBoot {
  buildInfo()
}

In smoke-tests (no graalvm native), it's not needed: #10515

Does this make sense?

FYI, the OtelSpringStarterSmokeTest test class can be executed both with a JVM runtime (./gradlew nativeTest) or a GraalVM native runtime (./gradlew test). The native mode is executed once a day with this Github action.

In smoke-tests (no graalvm native), it's not needed: #10515

I guess you are refering to this code change in #10515:

image

This code is related to a smoke test for which a Spring Boot application is package as a JAR and included in a Docker image. So, without buildInfo(), my guess is that the service name is retrieved from the JAR name (or perhaps otel.service.name or spring.application.name).

With OtelSpringStarterSmokeTest, no JAR is created during the test execution. And by adding buildInfo(), the service name is retrieved from the generated build info file.

@zeitlinger
Copy link
Member Author

removed the jar detector, because the manifest detector will always find a name and version for spring boot.

Comment on lines +48 to +50
*
* <p>Note: should not be used inside a spring application, where the spring.application.name is
* already available.
Copy link
Member

Choose a reason for hiding this comment

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

I didn't understand this note?

Copy link
Member Author

Choose a reason for hiding this comment

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

In spring, there's a resource provider that uses the always correct, pre-computed value from the spring environment.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, still not following

Copy link
Member Author

Choose a reason for hiding this comment

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

the spring boot starter uses SpringResourceProvider, which doesn't need to parse any application.yaml, etc. file, because when you run inside a sprig application, you can just ask the environment.

In this case, the lookup is here:

Copy link
Member

Choose a reason for hiding this comment

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

so SpringBootServiceNameDetector should only be used by the Java agent?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, exactly.

The provider will actually notice if a name found using shouldApply - so it's just a note about the intended usage.

Copy link
Member

Choose a reason for hiding this comment

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

could SpringBootServiceNameDetector be moved to a (new) spring-boot-resources/javaagent to make this extra clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done this move in #10621 - because this

PR moves SystemHelper to the common resource project (for other reasons).
Hope this makes sense.

@trask trask merged commit 1cd768e into open-telemetry:main Feb 21, 2024
47 checks passed
@zeitlinger zeitlinger deleted the spring-resource-detector branch February 22, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add the Spring resource detector to the OTel Spring starter
3 participants