-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add support for directly generating span graphs #745
Conversation
The following PR does the merge with origin/main. It just resolves the reported conflict in the |
Resolving conflicts from merging with Main
Thanks a bunch Mario! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #745 +/- ##
==========================================
+ Coverage 67.41% 67.91% +0.49%
==========================================
Files 102 103 +1
Lines 8853 9181 +328
==========================================
+ Hits 5968 6235 +267
- Misses 2445 2489 +44
- Partials 440 457 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀🙌🏻
@@ -41,6 +42,10 @@ const ( | |||
SpanMetricsCalls = "traces_spanmetrics_calls_total" | |||
SpanMetricsSizes = "traces_spanmetrics_size_total" | |||
TracesTargetInfo = "traces_target_info" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understood, Tempo generates traces_target_info
when the OTel Collector Service Graph Connector generates target_info
and we have a configuration flag to choose the one used (screenshot below).
@domasx2 do we have best practices here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean we may not need it? I simply went by what I saw Tempo generated for my cloud instance when using Alloy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed for sure. I think what @cyrille-leclerc wondering is wether you should align with Tempo metric generator metric names, or OTEL spanmetrics/ service graph metrics connector metric names. I don't have a strong opinion TBH, though would lean towards OTEL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh absolutely OTel then, just confirming, so if we renamed the collection as target_info
App O11y will continue to work, correct? I'll create a PR to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that span metric names change as well for Otel, instead of traces_span_*
it's duration_seconds_bucket
, duration_seconds_count
and duration_seconds_total
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as @domasx2 said, the question of OTel vs Tempo naming conventions. I would see value in adopting OTel conventions.
That being said, what's our recommendation if a customer is getting started combining Beyla with a bunch of app instrumented with OTel SDKs the simplest possible way, relying on Tempo metrics generation?
Sorry it's more complicated than we would like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with the Gang that it would be beneficial to run with the new otel default. Given that users can have more instrumentation than just a single beyla deployment, I would opt to make this configurable to be either Otel (default) OR tempo.
If it was flexible, people could rely on tempo generated metrics + the ones from Beyla.
I would also lean towards helping tempo to align with otel long-term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I think we have a consensus. We'll add an OTel format and make it default.
I opened a new issue to track this here #821
Some initial code for generation of span graphs directly from Beyla. The basic design is as follows:
TODO: