fix: add merge logic for exponential histogram when the temporality cumulative #1869
Conversation
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
xuan-cao-swi
requested review from
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd and
robertlaurin
as code owners
|
As an end-user what is the result of not having this fix? If we use exponential histograms, are they not accurate? |
|
This pr is for adding merge logic for cumulative aggregation. |
We use Alloy as the collector—how do I know whether it is able to handle histogram merges? I'm trying to understand whether I need this patch in order to use exponential histograms with the Ruby SDK today. |
|
I believe the alloy does merge the exponential histogram bucket (histogram.go) |
| def collect(start_time, end_time, data_points) | ||
| if @aggregation_temporality.delta? | ||
| # Set timestamps and 'move' data point values to result. | ||
| # puts "data_points.inspect: #{data_points.inspect}" |
There was a problem hiding this comment.
do we need to keep the puts?
| @previous_count = {} # 0 | ||
| @previous_zero_count = {} # 0 | ||
| @previous_scale = {} # nil | ||
| # @start_time_unix_nano = {} #nil |
|
@xuan-cao-swi - Could you send me a link to the Python implementation you used? I'd like to compare the two. |
|
@kaylareopelle - Yes, _merge function and when the merge is required |
| MINIMAL_SCALE = -10 | ||
| MAXIMAL_SCALE = 0 |
There was a problem hiding this comment.
Where is the MAXIMAL_SCALE constant used?
There was a problem hiding this comment.
It was used for scale check, but now it's not used since we check scale at exponential histogram class
| MINIMAL_SCALE = 1 | ||
| MAXIMAL_SCALE = 20 |
There was a problem hiding this comment.
Where are these constants used?
| end | ||
| end | ||
|
|
||
| # Integration tests moved from exponential_bucket_histogram_integration_test.rb |
There was a problem hiding this comment.
I don't understand this comment. Isn't this the file we're in?
There was a problem hiding this comment.
Sorry, I was meant to move python integration test into this file. Python exp histogram separates the integration test out of main exp histogram test case.
| # _(metric_data.sum).must_be_within_epsilon(previous_metric_data.sum, 1e-10) | ||
|
|
||
| # omit the test case for now before cumulative aggregation is tested |
There was a problem hiding this comment.
Do we still need to omit this case since the PR is fixing the merge logic for cumulative?
There was a problem hiding this comment.
No, the comment is outdated (removed)
| size | ||
| end | ||
|
|
||
| # checked, only issue is if @previous_scale is nil, then get_low_high may throw error |
There was a problem hiding this comment.
Would this error stop a user's application? Or is it rescued somewhere?
There was a problem hiding this comment.
Sorry for the confusion. @previous_scale[attributes] = current_scale if @previous_scale[attributes].nil? will ensure @previous_scale/previous_scale never be nil.
| end | ||
| end | ||
|
|
||
| # checked |
There was a problem hiding this comment.
Do we still need this?
…elemetry-ruby into exp-histogram-improve
3 participants
Description
Test
Test case are copied from python metrics sdk, and modified based on ruby metrics sdk data structure.