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.

Sign up for GitHub

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

Jump to bottom

Breaking changes to db.system attribute in OpenTelemetry Semantic Conventions v1.30.0 #4640

Open
matt-hensley opened this issue Jan 29, 2025 · 4 comments · May be fixed by #4656
Open

Breaking changes to db.system attribute in OpenTelemetry Semantic Conventions v1.30.0 #4640

matt-hensley opened this issue Jan 29, 2025 · 4 comments · May be fixed by #4656
Assignees
@iamrajiv
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@matt-hensley
Copy link

Is your feature request related to a problem? Please describe.

v1.30.0 of OpenTelemetry Semantic Conventions has two breaking changes to the db.system attribute. This will impact the ability for the metrics-generator to create virtual nodes in service graphs.

The breaking changes are:

  • Attribute renamed from db.system to db.system.name
  • List of constants for recognized databases updated

Describe the solution you'd like

db.system.name recognized as a peer attribute for service graph metrics generation.

Additional context

Diff between releases v1.29.0 and v1.30.0
@joe-elliott
Copy link
Member

It looks like we default to db.name. If we don't find that we try db.system. Do you think this new attribute should take priority over both of these?

So we would look for the following in order and use the first one we found?

  • db.system.name
  • db.name
  • dby.system

tempo/modules/generator/processor/servicegraphs/servicegraphs.go

Lines 295 to 306 in 4ac0715

// Check for db.name first. The dbName is set initially to maintain backwards compatbility.
if name, ok := processor_util.FindAttributeValue(string(semconv.DBNameKey), resourceAttr, span.Attributes); ok {
dbName = name
isDatabase = true
}
// Check for db.system only if we don't have db.name above
if !isDatabase {
if _, ok := processor_util.FindAttributeValue(string(semconv.DBSystemKey), resourceAttr, span.Attributes); ok {
isDatabase = true
}
}

@joe-elliott joe-elliott added good first issue Good for newcomers help wanted Extra attention is needed labels Jan 29, 2025
@matt-hensley
Copy link
Author

db.name was renamed to db.namespace in v1.26.0.

Looking at the current implementation, this ordering would preserve current behavior while supporting the new names:

  • db.name
  • db.namespace
  • db.system
  • db.system.name

@matt-hensley
Copy link
Author

matt-hensley commented Jan 29, 2025 •
edited
Loading

Blocked by:

@iamrajiv
Copy link

iamrajiv commented Feb 3, 2025

@matt-hensley I want to work on this. Can you assign it to me?

@iamrajiv iamrajiv linked a pull request Feb 3, 2025 that will close this issue
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@iamrajiv iamrajiv

Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(servicegraphs): Add support for db.system.name iamrajiv/tempo
3 participants
@matt-hensley @joe-elliott @iamrajiv