-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
de8cd2b
Add dns name resolution transformer
grcevski 15231eb
remove unused function
grcevski 0024cd9
Fix for matching same peer as host
grcevski b778a84
Add service graph series
grcevski 515be2e
some tweaks
grcevski 2e3dfc4
Finish logic for name resolver
grcevski 60249ed
Fix lints and formatting
grcevski 0121c9d
Add unit tests
grcevski 3311152
format and test fix
grcevski 2caee22
happy linter
grcevski 0675e52
Promscrape and integration tests
grcevski 8b2c153
Fix prom scrape bug
grcevski ddceaa7
Add docs
grcevski da4fa75
Make vale happy
grcevski fddd1db
More vale fixes
grcevski 156093c
Merge branch 'main' of github.com:grafana/ebpf-autoinstrument into sp…
mariomac b98ca5b
Merge pull request #4 from mariomac/span_graphs
grcevski ef52c18
Merge branch 'main' into span_graphs
grcevski 0f591bb
Add separate option as per review comments
grcevski 7c6a7e5
Merge branch 'main' into span_graphs
grcevski 85685a7
Merge branch 'main' into span_graphs
grcevski b517207
fix merge
grcevski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 generatestarget_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'sduration_seconds_bucket
,duration_seconds_count
andduration_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