-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove bound metric instruments from the API #2352
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2352 +/- ##
=======================================
- Coverage 73.6% 73.5% -0.2%
=======================================
Files 175 175
Lines 12419 12333 -86
=======================================
- Hits 9144 9065 -79
+ Misses 3041 3035 -6
+ Partials 234 233 -1
|
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.
+18 -518, best kind of PRs.
I like the thrust of the code, there needs a bit of attention to lint and changelog etc.
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.
+1 but needs a CHANGELOG entry
Thanks. I did't want to finish this PR until I had a few people agreeing I should. |
SGTM, looks like a worth while change. |
|
I was asked to take over getting this PR to the finish line. A new PR at #2399 fixes all of the outstanding issues. |
This demonstrates how much code we can remove by eliminating bound instruments. I believe this is worthwhile and I would rather focus on optimizing the code path for extracting baggage and the performance of
RecordBatch()
, not individual bound instrument APIs.Fixes #2351