search

spotify / heroic

The Heroic Time Series Database
https://spotify.github.io/heroic/
Apache License 2.0
848 stars 109 forks source link

Moves Tdigest stat computation to SharedGroups combine phase. (Part 1 of 2) #734

Closed ao2017 closed 3 years ago

ao2017 commented 3 years ago

Can I get a quick review for this PR the actual change and the integration tests will follow. This is mostly boilerplate changes in Bucket and AggregationSession implementations. The PR also add a new metric TdigestPoint, it is similar to group metric. It is used during the last phase of aggregation to merge tdigest from different cluster.

codecov[bot] commented 3 years ago

Codecov Report

Merging #734 (7a586e4) into master (5f757f5) will decrease coverage by 0.11%. The diff coverage is 32.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #734      +/-   ##
============================================
- Coverage     54.95%   54.83%   -0.12%     
- Complexity     3105     3106       +1     
============================================
  Files           739      742       +3     
  Lines         20147    20191      +44     
  Branches       1313     1313              
============================================
+ Hits          11071    11072       +1     
- Misses         8608     8652      +44     
+ Partials        468      467       -1
Impacted Files Coverage Δ Complexity Δ
...spotify/heroic/aggregation/simple/DeltaInstance.kt 81.81% <ø> (ø) 6.00 <0.00> (ø)
...eroic/aggregation/simple/DeltaPerSecondInstance.kt 82.35% <ø> (ø) 6.00 <0.00> (ø)
...ify/heroic/aggregation/simple/FilterAggregation.kt 47.82% <0.00%> (-2.18%) 5.00 <0.00> (ø)
...oic/aggregation/simple/MetricMappingAggregation.kt 54.54% <0.00%> (-2.60%) 5.00 <0.00> (ø)
...y/heroic/aggregation/simple/NotNegativeInstance.kt 77.41% <ø> (ø) 6.00 <0.00> (ø)
...a/com/spotify/heroic/aggregation/simple/Tdigest.kt 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...eroic/aggregation/simple/TdigestInstanceUtils.java 60.00% <0.00%> (ø) 3.00 <0.00> (?)
...com/spotify/heroic/aggregation/AbstractBucket.java 14.28% <0.00%> (-2.39%) 1.00 <0.00> (ø)
...java/com/spotify/heroic/aggregation/AnyBucket.java 72.72% <0.00%> (-16.17%) 4.00 <0.00> (ø)
...fy/heroic/aggregation/BucketAggregationInstance.kt 72.05% <0.00%> (-3.33%) 9.00 <0.00> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5f757f5...7a586e4. Read the comment docs.

lmuhlha commented 3 years ago

@ao2017 this looks alright to me, was just wondering since the quantiles are removed in the PR: is that to make the tdigest aggregation more generic and then you'll introduce default quantiles later? And why does TdigestStatInstanceUtils and testTDigestStatInstance continue to have stat in the name? What's the significance of removing it everywhere except there?

ao2017 commented 3 years ago

@ao2017 this looks alright to me, was just wondering since the quantiles are removed in the PR: is that to make the tdigest aggregation more generic and then you'll introduce default quantiles later? And why does TdigestStatInstanceUtils and testTDigestStatInstance continue to have stat in the name? What's the significance of removing it everywhere except there?

I moved the quantile to a different file since the computation is moving at a different level. That change will be included in the second PR. I am trying to keep the file count in this PR down.

I also changed the name of the Utils and class and the test.