-
Notifications
You must be signed in to change notification settings - Fork 41.1k
Add support for OpenTelemetry's service.namespace #44499
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much, @thecooldrop
I've left some comments for your consideration.
Please don't do anything until the Spring Boot team had a chance to review it.
I’ve opened #44494 to eliminate the duplications in OpenTelemetryAutoConfiguration
and OtlpMetricsPropertiesConfigAdapter
, consolidating everything in one place.
@@ -80,6 +80,9 @@ private Resource toResource(Environment environment, OpenTelemetryProperties pro | |||
.asMap(); | |||
attributes.computeIfAbsent("service.name", (key) -> getApplicationName(environment)); | |||
attributes.computeIfAbsent("service.group", (key) -> getApplicationGroup(environment)); | |||
// See https://github.com/spring-projects/spring-boot/issues/44411 for potential | |||
// information about deprecation of "service.group" attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that instead of reusing the existing getApplicationGroup
method, a new method,
getServiceNamespace
, can be introduced. This would make it possible to mark the
existing method with the @Deprecated
annotation and include appropriate javadoc.
private String getServiceNamespace(Environment environment) {
return environment.getProperty("spring.application.group");
}
/**
* @deprecated ...
*/
@Deprecated(since = "3.5.0", forRemoval = true)
private String getApplicationGroup(Environment environment) {
}
Using the @Deprecated
annotation would make it easier to ensure that the
service.group
property is deleted in future versions.
The same could be done in OtlpMetricsPropertiesConfigAdapter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. As per your note, will wait for input from Spring team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nosan, i think that's a good idea.
@@ -163,6 +163,17 @@ void shouldUseDefaultApplicationGroupIfApplicationGroupIsNotSet() { | |||
assertThat(createAdapter().resourceAttributes()).doesNotContainKey("service.group"); | |||
} | |||
|
|||
@Test | |||
void shouldUseServiceGroupForServiceNamespaceIfServiceGroupIsSet() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to include a similar test in OpenTelemetryAutoConfigurationTests
as well. Since OpenTelemetryAutoConfigurationTests
already contains a test for checking service.group
, you could potentially reuse that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking for that in exactly that class, but somehow I missed it in the mass.
Thanks for pointing this out. I will add this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that entire logic for determining the attributes, which are included is now contained in OpenTelemetryResourceAttributes
, I will place the tests only once in this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bit of a weird corner case. If service.group
is populated via OTEL_RESOURCE_ATTRIBUTES
env variable, and overrides spring.application.group
configuration from Environment
, then the question is if service.namespace
should be populated from OTEL_RESOURCE_ATTRIBUTES
as well, or from the service.application.group
.
My expectation would be to populate it from service.application.group
, as long as it is not listed in OTEL_RESOURCE_ATTRIBUTES
This choice is reflected in following test (note the similar, but different values for two entries):
@Test
void otelResourceAttributesShouldTakePrecedenceOverSpringApplicationGroupName() {
this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "service.group=spring-boot");
this.environment.setProperty("spring.application.group", "spring-boot-app");
assertThat(getAttributes()).hasSize(3)
.containsEntry("service.name", "unknown_service")
.containsEntry("service.group", "spring-boot")
.containsEntry("service.namespace", "spring-boot-app");
}
Could you please also rebase your changes on top of |
…to OpenTelemetryAutoconfiguration Signed-off-by: Vanio Begic <[email protected]>
…emetryResourceAttributes Signed-off-by: Vanio Begic <[email protected]>
Done |
See gh-44499 Signed-off-by: Vanio Begic <[email protected]>
Thanks @thecooldrop ! |
Add OTEL service attribute
service.namespace
. The value is set to same value asservice.group
currently. Since there is no specific environment variable to specify theservice.namespace
prescribed by OTEL, following chain is used to decide the value:service.group
is configured, then set it to same nameCloses #44411