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

Use unknown_service as default application name for OpenTelemetry #38219

Closed
wants to merge 1 commit into from

Conversation

lenin-jaganathan
Copy link

Fixes #38218

@mhalbritter
Copy link
Contributor

mhalbritter commented Nov 6, 2023

Hey, thanks for the issue and the PR.

In the OpenTelemetry SDK, they use this as the default:

  private static final Resource MANDATORY =
      create(Attributes.of(SERVICE_NAME, "unknown_service:java"));

I wonder if we should use our default at all. What do you think of this change?

	SdkTracerProvider otelSdkTracerProvider(Environment environment, ObjectProvider<SpanProcessor> spanProcessors,
			Sampler sampler) {
		String applicationName = environment.getProperty("spring.application.name");
		Resource springResource = Resource.create((applicationName != null) ? Attributes.of(ResourceAttributes.SERVICE_NAME, applicationName) : Attributes.empty());
// ...

That would only set the service name if the application name is set, otherwise it will use OTels SDK defaults.

@mhalbritter mhalbritter added the status: waiting-for-feedback We need additional information before we can continue label Nov 6, 2023
@lenin-jaganathan
Copy link
Author

Micrometer is not using OTEL SDK's so metrics will be using unknown_service as default. If service.name is not set in Resource bean, do you think it will cause inconsistencies between traces and metrics?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 6, 2023
@mhalbritter
Copy link
Contributor

Could be, that would be a question for the Micrometer team, I'll point them to that issue. Let's see what they say.

@mhalbritter
Copy link
Contributor

So, OTels default of unknown_service:java is wrong when the application is running as a native image, as the format of service.name is name:process.executable.name.

I lean towards making the Boot default unknown_service, which is correct when running in a native image and is also the same as in Micrometer.

The only thing I'm worried is that this is a breaking change if users haven't set spring.application.name. We have to decide in which version to ship this.

@mhalbritter mhalbritter added the for: team-attention An issue we'd like other members of the team to review label Nov 6, 2023
@wilkinsona
Copy link
Member

The only thing I'm worried is that this is a breaking change if users haven't set spring.application.name. We have to decide in which version to ship this.

I think it should be 3.2 at the very earliest and probably wait until 3.3 given where we are in 3.2's schedule.

@lenin-jaganathan
Copy link
Author

I believe for metric users moving from 3.1 to 3.2 will be a breaking change. As per this code and this code, I believe if service.name was not explicitly set in properties or OTEL_SERVICE_NAME is not an environmental variable 3.1 will use unknown_service as default. OpenTelemetry Resources were made common (between metrics and tracing) only in 3.2.

For tracing yes, it might be a breaking change given that in 3.1 "application" was used a fallback value.

@mhalbritter mhalbritter added type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Nov 6, 2023
@mhalbritter mhalbritter added this to the 3.x milestone Nov 6, 2023
@mhalbritter mhalbritter added the for: merge-with-amendments Needs some changes when we merge label Nov 6, 2023
@mhalbritter
Copy link
Contributor

We need to adjust the name in org.springframework.boot.actuate.autoconfigure.metrics.export.otlp.OtlpPropertiesConfigAdapter, too.

@mhalbritter mhalbritter modified the milestones: 3.x, 3.3.x Nov 7, 2023
@mhalbritter mhalbritter self-assigned this Jan 10, 2024
@mhalbritter mhalbritter modified the milestones: 3.3.x, 3.3.0-M1 Jan 10, 2024
@mhalbritter
Copy link
Contributor

Thank you very much and congratulations on your first contribution 🎉!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opentelemetry service.name default should align with OpenTelemetry Semantic Convention
4 participants