-
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
Add "inserted" metrics to processors #10372
Add "inserted" metrics to processors #10372
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10372 +/- ##
=======================================
Coverage 92.56% 92.56%
=======================================
Files 387 387
Lines 18254 18297 +43
=======================================
+ Hits 16896 16937 +41
- Misses 1014 1015 +1
- Partials 344 345 +1 ☔ 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.
I am okay with breaking API in one go, but the breaking change should appear in the API changelog.
I am not sure if there is any overlap with open-telemetry/oteps/pull/259 (I guess not in terms of the metric names, but possibly on the namespaces?). Is there a chance we would have to change these after that OTEP is finalized?
Thanks, I'll add a changelog - just wanted to agree on the direction before going further with it.
I think there is overlap and if the otep is accepted we will need to make quite a few changes. However, my understanding of the proposal is that all of the counts we currently have for processors, as well as these "inserted" counts would be relevant, though they would be grouped with others into new metrics. I think regardless of this PR we may end up deprecating the current metrics in favor of a new set, but both could coexist for a while. |
0716a03
to
b4bcc9f
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, will wait a few days in case others want to chime in
Link to tracking issue
Resolves #10353
Testing
Added equivalent testing to other processor metrics (accepted, refused, dropped).
Documentation
Metric documentation is autogenerated.
Open Question
My initial implementation includes a breaking change to
componenttest.TestTelemetry
which is public facing API. If we want to avoid an immediate breaking change in this test package, I would propose the following, which I can submit in a prerequisite PR:TestTelemetry.Check*
methods.TestTelemetry.CheckOneSpecificMetric
methods.