-
Notifications
You must be signed in to change notification settings - Fork 780
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
Histogram Record Method Benchmark #2754
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2754 +/- ##
=======================================
Coverage 83.75% 83.75%
=======================================
Files 250 250
Lines 8882 8882
=======================================
Hits 7439 7439
Misses 1443 1443
|
@@ -247,12 +247,18 @@ internal void Update(double number) | |||
case AggregationType.Histogram: | |||
{ | |||
int i; | |||
for (i = 0; i < this.histogramBuckets.ExplicitBounds.Length; i++) | |||
if (double.IsNaN(number)) |
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.
Interesting... 🤔 makes me wonder if NaN
should be an error case that we just log instead of recording.
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.
Ya, I'm not sure. I was trying to fix the Prometheus exporter test for PrometheusSerializerTests.HistogramNaN
since I was trying to compare by using the ==
operator and also double.NaN
returns False
for it appears all uses of <
, >
, ==
, 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.
Yea, doesn't make sense to me that we should increment the largest bucket... why not the smallest? or even a random one?
Interestingly, I checked the Java SDK, and it turns out they too increment the last bucket. Wonder if this is just accidental because I don't see anything in the spec.
Though I looked at that prometheus test and noted that NaN
is a valid value in the prometheus format.
Maybe something @reyang could shed some light on.
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.
The SDK Specification gives certain flexibility to the SDK implementations for perf consideration.
Prometheus Exporter spec (which is still Experimental) hasn't covered NaN
histogram yet, further clarifications are needed.
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.
If perf is what drives our decision then it would make sense to try and eliminate the if (double.IsNaN(number))
check.
If we settle on using Array.BinarySearch
then ~Array.BinarySearch(buckets, double.NaN)
equals 0. Seems it would be slightly more efficient to increment the first bucket in this case.
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.
Though, the bummer of aggregating a NaN
value is that it not only messes up one of the bucket counts, it also clobbers the sum.
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.
If perf is what drives our decision then it would make sense to try and eliminate the
if (double.IsNaN(number))
check.
+1
This is also called out in the spec:
It is unspecified how the SDK should handle the input limits. The SDK authors MAY leverage/follow the language runtime behavior for better performance, rather than perform a check on each value coming from the API.
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.
Though, the bummer of aggregating a
NaN
value is that it not only messes up one of the bucket counts, it also clobbers the sum.
In addition to NaN
, other values could result in NaN
as well, for example (note that currently histogram does not support negative value):
histogram.Record(double.PositiveInfinity);
histogram.Record(double.NegativeInfinity);
{ | ||
// If `number` is not a histogram bound, the result will be negative | ||
// The bitwise complement of the returned value is the insertion index | ||
i = Array.BinarySearch(this.histogramBuckets.ExplicitBounds, number); |
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 histogram with small number of bounds, this extra invocation could turn out to be slower.
We need to have a perf benchmark to show the numbers (especially for low buckets).
Refer to the prom-client and see how low/high buckets are handled differently:
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'll write a self-contained benchmark that compares the existing linear search implementation with the binary search one and run it with a range of boundary counts.
I agree that linear is likely faster for a small number of boundaries, but eventually the binary search will outperform it. Then I can a boundary number where anything less uses linear, and anything more uses binary.
I'll generate the random numbers so that each index gets an equal amount of finds.
Ex: If the sorted array is [ 20, 30, 40 ] then the random numbers will be generated like random.next(10, 50)
An array of all the results from performing 10,000 checks should looks similar to:
Value | Count |
---|---|
0 | 2500 |
1 | 2500 |
2 | 2500 |
3 | 2500 |
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.
We should add benchmarks for recording histogram values with different number of dimensions similar to how we have benchmarks for If it becomes too complicated, we could either remove the |
@mic-max I learned from some users of OpenTelemetry .NET that in some extreme cases they might be using more than 1k buckets. I guess we might want to revisit this and see if certain #2754 (comment) (e.g. I guess |
Changes
Benchmarking the Histogram.Record function for various number of explicitly provided bounds for both long and double values.
Aside: The reason this was added was since I was investigating whether changing the bucket finding algorithm from a linear one to a binary search one would improve the speed. It only did at ~100 boundaries. So opted not to make the change. Here is the results of that benchmark which is not included in this PR.
Current: Linear
Proposed: Binary
CHANGELOG.md
updated for non-trivial changes