-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[service/internal/graph] Measure telemetry as it is passed between pipeline components #12812
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
base: main
Are you sure you want to change the base?
[service/internal/graph] Measure telemetry as it is passed between pipeline components #12812
Conversation
b6bb02d
to
2f83f2b
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (82.98%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #12812 +/- ##
==========================================
- Coverage 91.67% 91.54% -0.13%
==========================================
Files 503 503
Lines 27819 28094 +275
==========================================
+ Hits 25502 25720 +218
- Misses 1828 1866 +38
- Partials 489 508 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
some initial thoughts, overall implementation seems very sane to me though. Thank you!
2f83f2b
to
052cffc
Compare
052cffc
to
362a610
Compare
fbed573
to
b789109
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 moving this work forward @djaglowski, just a few questions
Thanks for the reviews. #12817 implements a subset of this PR. If we can get that merged in first, I'll rebase to reduce the scope of this one substantially. |
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 think this looks really good 👍🏻
As shared in the other PR, I'd really like to see bytes counters too but this is a great start.
Subset of #12812 This internal package defines wrappers around consumers. These are useful for instrumenting the component graph, so that we can generate telemetry describing data as it is passed in between components. Currently, this supports only a single counter metric, but in the near future it can be enhanced to automatically capture multiple metrics (e.g. item count & size), and potentially spans and/or logs as well.
b789109
to
4afe6cd
Compare
4afe6cd
to
5bdd398
Compare
adec160
to
b66a5a8
Compare
testing this further, this appears to be the current behaviour in main, it looks like something broke in the last 20 commits |
Looks like things have been unhappy since this change: dc8e2dd |
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.
The PR LGTM. However, I'd like to get an agreement on #12916 before releasing this
I just tested and saw the same, internal metrics are broken at mainline head. |
Yes, reverting that commit fixes it http://github.com/open-telemetry/opentelemetry-collector/pull/12917 |
With #12933 merged, the metrics created by this PR are once again useless without the |
07a92cf
to
b54f142
Compare
b54f142
to
3b4dc35
Compare
3398805
to
c5e2fcf
Compare
@dmitryax @codeboten I will merge this by |
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, just one comment about the update of the tpm package, otherwise this looks good
e25097a
to
014f717
Compare
Depends on #12856
Resolves #12676
This is a reboot of #11311, incorporating metrics defined in the component telemetry RFC and attributes added in #12617.
The basic pattern is: