Skip to content

Conversation

@nosan
Copy link
Contributor

@nosan nosan commented Feb 28, 2025

Relates to #44394

Initially, I considered proposing these changes: main...nosan:spring-boot:gh-44394-1.
However, I eventually realized that it might be too much for this specific case.

This PR aims to eliminate duplications in OpenTelemetryAutoConfiguration and OtlpMetricsPropertiesConfigAdapter, consolidating everything into a single location.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 28, 2025
@nosan nosan marked this pull request as draft February 28, 2025 16:09
@nosan nosan changed the title Update OpenTelemetryResourceAttributes to exclude blank keys and trim keys/values Refine the handling of OpenTelemetry resource attributes Feb 28, 2025
@nosan nosan marked this pull request as ready for review February 28, 2025 20:23
@nosan nosan force-pushed the gh-44394 branch 4 times, most recently from 351254a to 2f59144 Compare March 1, 2025 08:55
@nosan nosan marked this pull request as draft March 1, 2025 09:27
@nosan nosan marked this pull request as ready for review March 1, 2025 10:52
@nosan
Copy link
Contributor Author

nosan commented Mar 1, 2025

Another option is to leave things as they are and simply add logic to ignore empty keys.

The attribute key MUST be a non-null and non-empty string.
Case sensitivity of keys is preserved. Keys that differ in casing are treated as distinct
keys.

main...nosan:spring-boot:44394

@mhalbritter mhalbritter added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 3, 2025
@mhalbritter mhalbritter modified the milestones: 3.5.0-M3, 3.5.x Mar 3, 2025
mhalbritter pushed a commit that referenced this pull request Mar 3, 2025
@mhalbritter mhalbritter modified the milestones: 3.5.x, 3.5.0-M3 Mar 3, 2025
@mhalbritter
Copy link
Contributor

Thanks @nosan !

@FyiurAmron
Copy link

FyiurAmron commented Oct 24, 2025

@mhalbritter @nosan sorry for chiming in so late, but I found this in the current code:

	/**
	 * Default value for service name if {@code service.name} is not set.
	 */
	private static final String DEFAULT_SERVICE_NAME = "unknown_service";

 - shouldn't it say (similarly what it said before)

	/**
	 * Default value for service name if neither {@code service.name} nor {@code spring.application.name} is set.
	 */
	private static final String DEFAULT_SERVICE_NAME = "unknown_service";

 - because, based on the codes and later comments, both would be checked?

attributes.computeIfAbsent("service.name", (key) -> getApplicationName());
// ...
	private String getApplicationName() {
		return this.environment.getProperty("spring.application.name", DEFAULT_SERVICE_NAME);
	}

@nosan
Copy link
Contributor Author

nosan commented Oct 24, 2025

I am not sure I understood your question. The current arrangement is:

  • If the user specifies service.name through management.opentelemetry.resource-attributes.service.name, it will be used.
  • If the user specifies the environment variable OTEL_SERVICE_NAME, it will be used.
  • If neither the environment variable nor the property is specified, then spring.application.name will be used.
  • If spring.application.name is not set, then unknown_service will be used.

@nosan
Copy link
Contributor Author

nosan commented Oct 24, 2025

In simple terms, if service.name is not set by any of the above methods, "unknown_service" will be used.

@FyiurAmron
Copy link

@nosan that seems like what I would expect. The current DEFAULT_SERVICE_NAME javadoc is lacking in that case, precisely because it lost the 2nd part of the OR (previously, it was the only part that it had). Yeah, I know it's a private const, but people debugging stuff read those docs :)

BTW you probably read the previous version of my comment, I've edited it to make it obvious what I was expecting here.

philwebb added a commit that referenced this pull request Oct 31, 2025
@philwebb
Copy link
Member

I've polished the javadoc in 22781fc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: task A general task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants