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

metrics-generator: include messaging systems and databases in service graphs #1576

Merged

Conversation

yvrhdn
Copy link
Member

@yvrhdn yvrhdn commented Jul 14, 2022

What this PR does:
Include databases and requests across messaging systems in service graphs metrics. We rely on the OpenTelemetry semantic conventions to detect when a span is involved in one of these systems.

Links:

Messaging system: span kind will be consumer and producer. We add connection_type="messaging_system" to the metrics.

Database: span kind will be client but there will be no matching server span (the database is most likely not instrumented). We copy the values from the client side and use that to populate the server latency. Downside: you can't track the client and server-side latency of a request.
We add connection_type="database" to the metrics.

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

Checklist

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

@yvrhdn
Copy link
Member Author

yvrhdn commented Jul 14, 2022

For testing: you can run this Tempo image in the intro-to-mlt demo https://github.com/grafana/intro-to-mlt/tree/kvrhdn/add-rabbitmq
I'll add proper test cases soon.

This will generate the following connections:
Screenshot 2022-07-14 at 19 38 49

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

Don't have a lot to comment. It looks good 👍

Comment on lines +96 to +103
edge := newEdge(key, s.ttl)
update(edge)

if edge.isComplete() {
s.onComplete(edge)
return true, nil
}

Copy link
Member

Choose a reason for hiding this comment

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

Nice improvement 👍

@yvrhdn
Copy link
Member Author

yvrhdn commented Jul 20, 2022

I'll update the corresponding docs in a separate PR since I've a couple of changes pending and it's just going to clutter this PR.

@yvrhdn yvrhdn marked this pull request as ready for review July 20, 2022 14:49
@yvrhdn yvrhdn requested a review from mapno July 20, 2022 15:04
@yvrhdn yvrhdn mentioned this pull request Jul 20, 2022
3 tasks
Comment on lines 155 to 157
case v1_trace.Span_SPAN_KIND_PRODUCER:
connectionType = store.MessagingSystem
fallthrough
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add a comment here to emphasise why the producer case was placed before client and similarly for consumer <> server.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa, I've never seen fallthrough before. Neat.

@yvrhdn yvrhdn merged commit 51a26cd into grafana:main Aug 2, 2022
@yvrhdn yvrhdn deleted the kvrhdn/service-graphs-track-queus-and-databases branch August 2, 2022 14:45
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.

Include spans with span kind producer/consumer in service graphs
4 participants