-
Notifications
You must be signed in to change notification settings - Fork 503
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
MetricRefactor - Part1 - Move AttributeSet inside aggregations #1977
MetricRefactor - Part1 - Move AttributeSet inside aggregations #1977
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1977 +/- ##
=======================================
- Coverage 74.9% 74.8% -0.1%
=======================================
Files 122 122
Lines 20411 20429 +18
=======================================
+ Hits 15296 15298 +2
- Misses 5115 5131 +16 ☔ 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.
Looks good! Left some suggestions.
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. These seems to be all internal changes, so no changelog required.
Part 1 of metric sdk refactor to achieve perf gains and no-heap-allocation. The heap allocation and perf cost is due to the usage of
AttributeSet
which clones and stores the incoming slice of KV pairs, after sorting + de-duplicating, which can be eliminated.This PR modifies traits to only need
&[KeyValue]
(which is what API accepts from user) instead ofAttributeSet
. There is no perf gain in this PR, as theAttributeSet
is still constructed inside each aggregations. This is done to keep the scope of PR very small, and easy to review.In the next PR, Sum aggregation will be targeted to achieve the perf gains and no-heap alloc, later extending to other instruments.