-
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
Add PoC implementation of Sum aggregator #2973
Conversation
Separate the duties of aggregation and maintaining state across aggregation periods.
Add back when filling in structures.
This is an implementation detail.
Codecov Report
@@ Coverage Diff @@
## new_sdk/main #2973 +/- ##
==============================================
+ Coverage 74.0% 74.5% +0.4%
==============================================
Files 114 125 +11
Lines 7930 8419 +489
==============================================
+ Hits 5874 6278 +404
- Misses 1921 2002 +81
- Partials 135 139 +4
|
sdk/metric/internal/sum.go
Outdated
type sumAgg[N int64 | float64] struct { | ||
// zero value used for the base of all new sums. | ||
newFunc NewAtomicFunc[N] | ||
|
||
// map[attribute.Set]Atomic[N] | ||
current sync.Map | ||
} |
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 we are going to have better results if we just use a single lock over two atomic operations (both a load from the map, and a load from the register). I will put together a branch that would show this off.
type sumAgg[N int64 | float64] struct { | |
// zero value used for the base of all new sums. | |
newFunc NewAtomicFunc[N] | |
// map[attribute.Set]Atomic[N] | |
current sync.Map | |
} | |
type sumAgg[N int64 | float64] struct { | |
mu sync.Mutex | |
values map[*attribute.Set]N | |
} |
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.
Here is what the change looks like: /~https://github.com/MadVikingGod/opentelemetry-go/blob/mvg/new_sdk/impl-sum/sdk/metric/internal/sum.go
Also check out the benchmarks I added.
Results: https://gist.github.com/MadVikingGod/1d48ff07ee87e3c66acdc33b895d6b3d
Changed to a simpler sum.
Will recreate based on the merged form of #2954 |
Based on #2954
Resolves #2972