-
Notifications
You must be signed in to change notification settings - Fork 453
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 collect methods on ValueMap #2267
Conversation
c699d05
to
60e17bf
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2267 +/- ##
=======================================
- Coverage 79.5% 79.4% -0.2%
=======================================
Files 121 121
Lines 20978 20787 -191
=======================================
- Hits 16685 16512 -173
+ Misses 4293 4275 -18 ☔ View full report in Codecov by Sentry. |
60e17bf
to
8d56bd2
Compare
069181e
to
66cb25a
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.
Great work!
There are two newer kinds of allocations happening with these changes as mentioned in #2267 (comment) and #2267 (comment). These can be avoided. I had a chat with @fraillt on Slack and he agreed to address these in his upcoming PRs.
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! Looking forward to the follow up improvements.
Design discussion issue #2145
Changes
Mostly extract
DataPoint
collection logic from specific metrics implementations intoValueMap
undercollect_readonly
andcollect_and_reset
functions.I tried to preserve existing logic with few exceptions:
There are a lot more things that could be improved, but with this change it will become more convenient/simpler to experiment/implement further improvements in the future.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes