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 logical name 'peer.service' for the service map #178

Open
jdehaan opened this issue Sep 21, 2023 · 1 comment
Open

Use logical name 'peer.service' for the service map #178

jdehaan opened this issue Sep 21, 2023 · 1 comment

Comments

@jdehaan
Copy link

jdehaan commented Sep 21, 2023

We noticed that with several redis services setup (session tokens, cache, ...), the service map displayed only displays a single "redis" bubble in signoz UI. According to the opentelemetry spec, a way to specify the logical instances of a service from the client perspective (application being instrumented) is to use peer.service.

As far as I understand the code the service map is built from information which comes from the query service which in turn becomes its information from the clickhouse database.

Then I drilled down to the code here:

tagMap['db.system'] as dest,

Shouldn't this use primarily the peer.service attribute instead of db.system ?

IMHO dest should be changed to be

  • peer.service if set
  • db.system otherwise (fallback)

Tested changes working:

-- in dependency_graph_minutes_db_calls_mv_v2
  coalesce(tagMap['peer.service'], tagMap['db.system']) as dest,

-- in dependency_graph_minutes_messaging_calls_mv_v2
  coalesce(tagMap['peer.service'], tagMap['messaging.system']) as dest,

What is your opinion?

PS: I tried the above code change and matching migrations added. I did a brute force replacement of the materialized view, maybe there is a more subtle way by updating....

@jdehaan
Copy link
Author

jdehaan commented Sep 22, 2023

I think now I missed updating the migrations...

Tried it out with really brutal migrations, delete and recreate the materialized views. It works as intended, although the UI now seems not adapted to display service maps of unrelated service sets. I mean different groups of services not connected to each other themselves. But basically that yieds data-wise the expected results for me.

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

No branches or pull requests

1 participant