-
Notifications
You must be signed in to change notification settings - Fork 599
AttributeSet cleanup, better perf for overflows #2313
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
AttributeSet cleanup, better perf for overflows #2313
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2313 +/- ##
=======================================
- Coverage 79.5% 79.5% -0.1%
=======================================
Files 123 123
Lines 21282 21258 -24
=======================================
- Hits 16927 16907 -20
+ Misses 4355 4351 -4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| // leveraging a ThreadLocal vec. | ||
| let mut sorted = attributes.to_vec(); | ||
| sorted.sort_unstable_by(|a, b| a.key.cmp(&b.key)); | ||
| sorted.dedup_by(|a, b| a.key == b.key); |
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.
just a nit change in behavior of dedup it seems - earlier with (AttributeSet) last occurrence of key was retained (because of rev call), now the first occurrence is retained.
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.
For an API like SetAttribute, it totally makes sense to overwrite the existing one and retain the new one. Spec also recommends that. But for an existing collection - dont know if one is better than the other. This clearly indicates an issue at instrumentation/user side, so I think we don't need to make any promise here about which one wins.
Also prometheus deals with this by taking all values and concatenating then with comma separation.
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 spotted same thing:) Now it actually is undefined, because sort order is unstable.
I tried to find the answer in the spec, and I couldnt find any restrictions... it says:
The enforcement of uniqueness may be performed in a variety of ways as it best fits the limitations of the particular implementation.
So probably this works just fine.
However, I have wrote this impl, which is also free of allocations.
vec.sort_by(|a, b| a.key.cmp(&b.key));
// we cannot use vec.dedup_by because it will remove last duplicate not first
if vec.len() > 1 {
let mut i = vec.len() - 1;
while i != 0 {
let is_same = unsafe { vec.get_unchecked(i - 1).key == vec.get_unchecked(i).key };
if is_same {
vec.remove(i - 1);
}
i -= 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.
However, I have wrote this impl, which is also free of allocations.
Allocation is not in the sorting/de-dup logic, but just outside of that in the vec! creation! I briefly tried thread-local, and it can solve that issue also.
With #2282 merged, the last known usage of AttributeSet is removed. This PR removes it, and makes an improvement to the dedup+sort by doing a single vec! allocation and sorting first, and then de-duping in-place. The overflow benchmark improves 30%, but still allocates the vec. Left a TODO to make this also go away by leveraging a threadlocal vec.
Why is it important to get the allocation to be zero for the overflow scenario?
Imagine OTel is used by a web application to report metrics, and one of the dimension is obtained from the request. If a malicious user DDoS the application by passing unique values with each request, we certainly don't want OTel Metrics to cause OOM and crash the app. This is already handled as we have CardinalityLimit enforced. But we still allocate a vec and drop it right after - this is something we can avoid. Once we know the limit is hit, then OTel SDK should be operating without any heap allocation, wherever feasible.
There are other issues in this area - especially about dropping read lock and acquiring write lock - this can be avoided too, as once we know both incoming and sorted order fails and cardinality limit is reached - then we can avoid write lock completely. This require some more refactoring, so will be a separate PR.