-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
RFC - Pipeline Component Telemetry #11406
base: main
Are you sure you want to change the base?
RFC - Pipeline Component Telemetry #11406
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11406 +/- ##
=======================================
Coverage 91.61% 91.61%
=======================================
Files 443 443
Lines 23770 23770
=======================================
Hits 21776 21776
Misses 1620 1620
Partials 374 374 ☔ View full report in Codecov by Sentry. |
99e3086
to
5df52e1
Compare
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.
Thanks for opening this as a RFC @djaglowski!
Based on some offline feedback, I've broadened the scope of the RFC, while simultaneously clarifying that it is intended to evolve as we identify additional standards. |
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.
a few questions, I really like this proposal overall :)
Some of my comments might have been discussed before, in which case, feel free to ignore me and just mark the items as resolved. |
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.
Approved with comments.
Co-authored-by: Alex Boten <[email protected]>
Co-authored-by: Damien Mathieu <[email protected]>
Co-authored-by: William Dumont <[email protected]>
Co-authored-by: Evan Bradley <[email protected]>
7d7a75b
to
a7a15e5
Compare
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.
LGTM
This sets the level of all metrics that where not previously stabilized as alpha. Since many of these metrics will change as a result of open-telemetry#11406, it made sense to me to set their stability as alpha. Signed-off-by: Alex Boten <[email protected]>
This has enough approvals and has entered the 'final comment period'. I will merge this on 2024-11-27 if nobody blocks before. cc @open-telemetry/collector-approvers |
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.
There are a couple of things to iron out, but I'm giving my approval already, as those are details that could be part of a follow-up PR. I don't want to block progress on dependent tasks because of those two rather small points.
This sets the level of all metrics that where not previously stabilized as alpha. Since many of these metrics will change as a result of #11406, it made sense to me to set their stability as alpha. --------- Signed-off-by: Alex Boten <[email protected]>
I believe all feedback has been addressed. #11743 represents two followup items raised by @jpkrohling, but I believe the RFC is clear that some changes are anticipated. |
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.
Thanks @djaglowski
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 approved this before, but I'll approve again, to make it explicit that I'm OK with the latest state of this PR.
This PR adds a RFC for normalized telemetry across all pipeline components. See #11343
edit by @mx-psi: