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

feat: Allow spans attributes to override service.name #277

Closed
wants to merge 3 commits into from

Conversation

MikeGoldsmith
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith commented Sep 23, 2024

Which problem is this PR solving?

We currently only use the service.name value when it's set at the resource level and ignore it at the span attributes level. This change allows the service.name to be set via span attribute. The same service.name rules apply to both levels.

The precedence order is span attribute > resource attribute.

Short description of the changes

  • Allow service.name to be used for dataset from span attribute, overriding the resource level value if set
  • Add unit tests

@MikeGoldsmith MikeGoldsmith self-assigned this Sep 23, 2024
@MikeGoldsmith MikeGoldsmith requested a review from a team as a code owner September 23, 2024 14:27
@MikeGoldsmith MikeGoldsmith added the type: enhancement New feature or request label Sep 23, 2024
Base automatically changed from mike/remove-batch-size to main September 23, 2024 14:28
…vel-service-name

# Conflicts:
#	otlp/traces_test.go
@@ -323,7 +323,7 @@ func isInstrumentationLibrary(libraryName string) bool {
return false
}

func getDataset(ri RequestInfo, attrs map[string]interface{}) string {
func getDataset(ri RequestInfo, attrs map[string]interface{}, defaultServiceName string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

The parameter name shadowing a constant is a bit tricky. Can we name this parameter something that's different from the constant?

@JamieDanielson JamieDanielson added the breaking-change Prefer 'version: bump major', but use this for breaking changes that don't bump major. label Oct 22, 2024
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

My main concern with this is the breaking nature of the change. For many it's probably fine, but if anyone has a span with a service.name attribute that perhaps they've forgotten about or set in the past and forgotten to update, then this would result in their telemetry going to a different place than what is currently set in the resource attribute.

As I understand it, the general idea of this change is intended to help folks who set a span attribute instead of a resource attribute to make sure their data is set to the proper service. Is it possible to do a check for unknown_* prefix before overwriting?

Theoretically this should only come up for new services being instrumented, after which they are fixed/updated to use resource attributes once the problem is discovered. By allowing the use of a span attribute, we're hiding the problem from the user and that could still have unexpected consequences outside of this (e.g. if they later use a collector processor that expects a service.name resource attribute they will have to fix it there anyway).

@MikeGoldsmith
Copy link
Contributor Author

I'm gonna close for now. We can revisit in the future if we still want to go this route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Prefer 'version: bump major', but use this for breaking changes that don't bump major. type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants