Service name auto discovery docs and polishing #2440
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
felixbarny
added
the
await-release
| public static void afterInitPropertySources(@Advice.This WebApplicationContext applicationContext) { | ||
| // avoid having two service names for a standalone jar | ||
| // one based on Implementation-Title and one based on spring.application.name | ||
| if (!ServiceInfo.autoDetected().isMultiServiceContainer()) { |
There was a problem hiding this comment.
Some background:
The risk of that is that if we don't properly detect that we're running on an application server/servlet container. In that case, we wouldn't be able to determine the service name based on spring.application.name. That's why I didn't add the check in Tracer.overrideServiceInfoForClassLoader but made it specific to this instrumentation. This way, we at least can still override based on the servlet context path and the display-name. It's quite unlikely that a standalone application that's using embedded Tomcat, for example, sets a display-name or even a servlet context path. Therefore, if any of these are set, that's a pretty good indication that we're running in a container and not in a standalone jar.
| public static void afterInitPropertySources(@Advice.This WebApplicationContext applicationContext) { | ||
| // avoid having two service names for a standalone jar | ||
| // one based on Implementation-Title and one based on spring.application.name | ||
| if (!ServiceInfo.autoDetected().isMultiServiceContainer()) { |
There was a problem hiding this comment.
I think we could still honor the spring application name here in case the manifest entry is not set.
- If the manifest entry is available, we make
isMultiServiceContainer()returnfalseand thenServiceInfo.autoDetected()will match what was read in the manifest - If the manifest entry is not available, we should still set it, so in the case of our test application it should still work as expected.
There was a problem hiding this comment.
Clarified this through an offline discussion: we have another fallback here for the ServiceInfo.autoDetected() that relies on the .jar name. Unfortunately when running in the IDE the test app is not packaged and thus not available.
| assertThat(transaction.getTraceContext().getServiceName()).isEqualTo("spring-boot-test"); | ||
| // as this test runs in a standalone application and not in a servlet container, | ||
| // the service.name will not be overwritten for the webapp class loader based on spring.application.name | ||
| assertThat(transaction.getTraceContext().getServiceName()).isNull(); |
There was a problem hiding this comment.
We should be able to still support this in the case where the auto-detected value is not read from manifest (see previous comment).
felixbarny
removed
the
await-release
2 participants
What does this PR do?
Implementation-TitleandImplementation-Versionmanifest entries with mavenspring.application.name.Checklist
[ ] Added an API method or config option? Document in which version this will be introduced