Skip to content

feat(service-graphs): add database_name_attributes config to service graph processor #5398

Merged
joe-elliott merged 7 commits intografana:mainfrom
KyriosGN0:db
Dec 12, 2025
Merged

feat(service-graphs): add database_name_attributes config to service graph processor #5398
joe-elliott merged 7 commits intografana:mainfrom
KyriosGN0:db

Conversation

@KyriosGN0
Copy link
Copy Markdown
Contributor

@KyriosGN0 KyriosGN0 commented Jul 13, 2025

Signed-off-by: AvivGuiser avivguiser@gmail.com

What this PR does:

this PR introduces a array of database name attributes that the service graph processor looks for, if it finds one of them it uses them for edge node name, this is similiar to the way the service graph processor behaves in the otel collector.
the list is currently empty but if we want we can set the db.name as default (like in the collector) but that would require changes to the other test

Which issue(s) this PR fixes:
Fixes #5243

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 2, 2025

This PR has been automatically marked as stale because it has not had any activity in the past 60 days.
The next time this stale check runs, the stale label will be removed if there is new activity. This pull request will be closed in 15 days if there is no new activity.
Please apply keepalive label to exempt this Pull Request.

@github-actions github-actions Bot added the stale Used for stale issues / PRs label Oct 2, 2025
@KyriosGN0
Copy link
Copy Markdown
Contributor Author

Not stale

@github-actions github-actions Bot removed the stale Used for stale issues / PRs label Oct 3, 2025
Comment thread modules/generator/processor/servicegraphs/servicegraphs.go Outdated
# Enables additional labels for services and virtual nodes.
[enable_virtual_node_label: <bool> | default = false]

# list of attribute names used to identify the database name from span attributes. if not set the order is peer.service -> server.address -> network.peer.address -> db.name
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
# list of attribute names used to identify the database name from span attributes. if not set the order is peer.service -> server.address -> network.peer.address -> db.name
# List of attribute names used to identify the database name from span attributes. If it isn't set, the order is peer.service -> server.address -> network.peer.address -> db.name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done!

Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Copy link
Copy Markdown
Collaborator

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

changelog and we're good merge!

thanks, friend.

cfg.EnableMessagingSystemLatencyHistogram = false

cfg.DatabaseNameAttributes = []string{
string(semconvnew.DBNamespaceKey),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i nearly commented that these are out of order with the previous checks, but these are in fact correct. the previous logic is just awfully convoluted. nice catch

Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0
Copy link
Copy Markdown
Contributor Author

@joe-elliott added changelog entry, Thanks!

Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@KyriosGN0
Copy link
Copy Markdown
Contributor Author

Weird, the linter complains about files I didn't touch

@joe-elliott
Copy link
Copy Markdown
Collaborator

Yeah, I've seen this happen before after rebase. Humorously the files that are failing lint have actually been deleted from main.

I'm going to drop the lint requirement real quick and merge.

@joe-elliott joe-elliott merged commit d1ec3c6 into grafana:main Dec 12, 2025
51 of 54 checks passed
oleg-kozlyuk-grafana pushed a commit to oleg-kozlyuk-grafana/tempo that referenced this pull request Dec 13, 2025
…graph processor (grafana#5398)

* add config and check if config exist and use it first if it exist

Signed-off-by: AvivGuiser <avivguiser@gmail.com>

* add test and update docs

Signed-off-by: AvivGuiser <avivguiser@gmail.com>

* update the manifest.md

Signed-off-by: AvivGuiser <avivguiser@gmail.com>

* cleanup and fix comments

Signed-off-by: AvivGuiser <avivguiser@gmail.com>

* pass 3 default attributes to database_name_attributes

Signed-off-by: AvivGuiser <avivguiser@gmail.com>

* add changelog entry

Signed-off-by: AvivGuiser <avivguiser@gmail.com>

* update manifest.md

Signed-off-by: AvivGuiser <avivguiser@gmail.com>

---------

Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update servicegraph database request handling

3 participants