Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Simplify the histogram aggregator #4370

Merged
merged 2 commits into from
Jul 27, 2023
Merged

Simplify the histogram aggregator #4370

merged 2 commits into from
Jul 27, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Jul 26, 2023 •
edited
Loading

Part of #4220

  • Accept a destination type for the underlying delta or cumulative. This allows memory reuse for collections which adds a considerable optimization.
  • Simplify the integration testing of the histogram aggregate.
  • Update benchmarking.
  • Remove old aggregator types and testing.

Benchmarking

Measure

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric/internal/aggregate
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                                           │   old.txt   │              new.txt               │
                                           │   sec/op    │   sec/op     vs base               │
Histogram/Int64/Cumulative/1/Measure-8       95.17n ± 1%   96.08n ± 1%  +0.95% (p=0.023 n=10)
Histogram/Int64/Cumulative/10/Measure-8      1.192µ ± 5%   1.151µ ± 5%       ~ (p=0.392 n=10)
Histogram/Int64/Cumulative/100/Measure-8     11.66µ ± 2%   11.71µ ± 7%       ~ (p=0.247 n=10)
Histogram/Int64/Delta/1/Measure-8            109.1n ± 3%   107.0n ± 1%       ~ (p=0.210 n=10)
Histogram/Int64/Delta/10/Measure-8           1.239µ ± 5%   1.264µ ± 6%       ~ (p=0.984 n=10)
Histogram/Int64/Delta/100/Measure-8          12.41µ ± 4%   12.26µ ± 4%       ~ (p=0.796 n=10)
Histogram/Float64/Cumulative/1/Measure-8     110.3n ± 3%   109.9n ± 2%       ~ (p=0.839 n=10)
Histogram/Float64/Cumulative/10/Measure-8    1.244µ ± 8%   1.239µ ± 3%       ~ (p=0.239 n=10)
Histogram/Float64/Cumulative/100/Measure-8   12.29µ ± 6%   11.97µ ± 6%       ~ (p=0.075 n=10)
Histogram/Float64/Delta/1/Measure-8          109.7n ± 4%   108.2n ± 5%       ~ (p=0.138 n=10)
Histogram/Float64/Delta/10/Measure-8         1.260µ ± 3%   1.244µ ± 5%       ~ (p=0.725 n=10)
Histogram/Float64/Delta/100/Measure-8        12.38µ ± 2%   12.29µ ± 3%       ~ (p=0.393 n=10)
geomean                                      1.167µ        1.157µ       -0.85%

                                           │   old.txt    │               new.txt               │
                                           │     B/op     │    B/op     vs base                 │
Histogram/Int64/Cumulative/1/Measure-8       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Cumulative/10/Measure-8      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Cumulative/100/Measure-8     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/1/Measure-8            0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/10/Measure-8           0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/100/Measure-8          0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/1/Measure-8     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/10/Measure-8    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/100/Measure-8   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/1/Measure-8          0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/10/Measure-8         0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/100/Measure-8        0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                 ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                                           │   old.txt    │               new.txt               │
                                           │  allocs/op   │ allocs/op   vs base                 │
Histogram/Int64/Cumulative/1/Measure-8       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Cumulative/10/Measure-8      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Cumulative/100/Measure-8     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/1/Measure-8            0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/10/Measure-8           0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/100/Measure-8          0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/1/Measure-8     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/10/Measure-8    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/100/Measure-8   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/1/Measure-8          0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/10/Measure-8         0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/100/Measure-8        0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                 ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

ComputeAggregation

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric/internal/aggregate
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                                                      │   old.txt    │               new.txt               │
                                                      │    sec/op    │   sec/op     vs base                │
Histogram/Int64/Cumulative/1/ComputeAggregation-8        369.2n ± 5%   215.7n ± 4%  -41.60% (p=0.000 n=10)
Histogram/Int64/Cumulative/10/ComputeAggregation-8      1698.5n ± 3%   747.8n ± 3%  -55.98% (p=0.000 n=10)
Histogram/Int64/Cumulative/100/ComputeAggregation-8     15.837µ ± 1%   9.346µ ± 1%  -40.98% (p=0.000 n=10)
Histogram/Int64/Delta/1/ComputeAggregation-8             498.2n ± 2%   338.8n ± 3%  -32.00% (p=0.000 n=10)
Histogram/Int64/Delta/10/ComputeAggregation-8            2.546µ ± 4%   1.623µ ± 5%  -36.26% (p=0.000 n=10)
Histogram/Int64/Delta/100/ComputeAggregation-8           23.07µ ± 2%   15.00µ ± 4%  -34.97% (p=0.000 n=10)
Histogram/Float64/Cumulative/1/ComputeAggregation-8      399.4n ± 4%   241.2n ± 6%  -39.59% (p=0.000 n=10)
Histogram/Float64/Cumulative/10/ComputeAggregation-8    1860.0n ± 7%   771.8n ± 5%  -58.51% (p=0.000 n=10)
Histogram/Float64/Cumulative/100/ComputeAggregation-8   16.950µ ± 1%   9.360µ ± 0%  -44.78% (p=0.000 n=10)
Histogram/Float64/Delta/1/ComputeAggregation-8           515.2n ± 2%   342.4n ± 5%  -33.53% (p=0.000 n=10)
Histogram/Float64/Delta/10/ComputeAggregation-8          2.623µ ± 3%   1.636µ ± 1%  -37.63% (p=0.000 n=10)
Histogram/Float64/Delta/100/ComputeAggregation-8         24.00µ ± 3%   15.16µ ± 2%  -36.85% (p=0.000 n=10)
geomean                                                  2.648µ        1.544µ       -41.67%

                                                      │    old.txt    │               new.txt                │
                                                      │     B/op      │     B/op      vs base                │
Histogram/Int64/Cumulative/1/ComputeAggregation-8         264.00 ± 0%     72.00 ± 0%  -72.73% (p=0.000 n=10)
Histogram/Int64/Cumulative/10/ComputeAggregation-8        2336.0 ± 0%     288.0 ± 0%  -87.67% (p=0.000 n=10)
Histogram/Int64/Cumulative/100/ComputeAggregation-8     20.391Ki ± 0%   2.391Ki ± 0%  -88.28% (p=0.000 n=10)
Histogram/Int64/Delta/1/ComputeAggregation-8              240.00 ± 0%     48.00 ± 0%  -80.00% (p=0.000 n=10)
Histogram/Int64/Delta/10/ComputeAggregation-8            2096.00 ± 0%     48.00 ± 0%  -97.71% (p=0.000 n=10)
Histogram/Int64/Delta/100/ComputeAggregation-8          18480.00 ± 0%     48.00 ± 0%  -99.74% (p=0.000 n=10)
Histogram/Float64/Cumulative/1/ComputeAggregation-8       264.00 ± 0%     72.00 ± 0%  -72.73% (p=0.000 n=10)
Histogram/Float64/Cumulative/10/ComputeAggregation-8      2336.0 ± 0%     288.0 ± 0%  -87.67% (p=0.000 n=10)
Histogram/Float64/Cumulative/100/ComputeAggregation-8   20.391Ki ± 0%   2.391Ki ± 0%  -88.28% (p=0.000 n=10)
Histogram/Float64/Delta/1/ComputeAggregation-8            240.00 ± 0%     48.00 ± 0%  -80.00% (p=0.000 n=10)
Histogram/Float64/Delta/10/ComputeAggregation-8          2096.00 ± 0%     48.00 ± 0%  -97.71% (p=0.000 n=10)
Histogram/Float64/Delta/100/ComputeAggregation-8        18480.00 ± 0%     48.00 ± 0%  -99.74% (p=0.000 n=10)
geomean                                                  2.168Ki          133.3       -93.99%

                                                      │  old.txt   │              new.txt               │
                                                      │ allocs/op  │ allocs/op   vs base                │
Histogram/Int64/Cumulative/1/ComputeAggregation-8       4.000 ± 0%   3.000 ± 0%  -25.00% (p=0.000 n=10)
Histogram/Int64/Cumulative/10/ComputeAggregation-8      13.00 ± 0%   12.00 ± 0%   -7.69% (p=0.000 n=10)
Histogram/Int64/Cumulative/100/ComputeAggregation-8     103.0 ± 0%   102.0 ± 0%   -0.97% (p=0.000 n=10)
Histogram/Int64/Delta/1/ComputeAggregation-8            3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
Histogram/Int64/Delta/10/ComputeAggregation-8           3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
Histogram/Int64/Delta/100/ComputeAggregation-8          3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
Histogram/Float64/Cumulative/1/ComputeAggregation-8     4.000 ± 0%   3.000 ± 0%  -25.00% (p=0.000 n=10)
Histogram/Float64/Cumulative/10/ComputeAggregation-8    13.00 ± 0%   12.00 ± 0%   -7.69% (p=0.000 n=10)
Histogram/Float64/Cumulative/100/ComputeAggregation-8   103.0 ± 0%   102.0 ± 0%   -0.97% (p=0.000 n=10)
Histogram/Float64/Delta/1/ComputeAggregation-8          3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
Histogram/Float64/Delta/10/ComputeAggregation-8         3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
Histogram/Float64/Delta/100/ComputeAggregation-8        3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.000 n=10)
geomean                                                 7.245        5.555       -23.33%

@MrAlias MrAlias added Skip Changelog PRs that do not require a CHANGELOG.md entry pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Jul 26, 2023
@codecov
Copy link

codecov bot commented Jul 26, 2023 •
edited
Loading

Codecov Report

Merging #4370 (31cb92a) into main (4f0d73c) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4370     +/-   ##
=======================================
- Coverage   83.4%   83.4%   -0.1%     
=======================================
  Files        184     184             
  Lines      14383   14371     -12     
=======================================
- Hits       11998   11986     -12     
  Misses      2157    2157             
  Partials     228     228
Files Changed Coverage Δ
sdk/metric/internal/aggregate/aggregate.go 100.0% <100.0%> (ø)
sdk/metric/internal/aggregate/histogram.go 100.0% <100.0%> (ø)

@MrAlias MrAlias force-pushed the simp-hist branch 2 times, most recently from bae7b5b to c89ae8e Compare July 26, 2023 20:41
@MrAlias MrAlias marked this pull request as ready for review July 26, 2023 20:49
pellared
pellared approved these changes Jul 27, 2023
View reviewed changes
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet 👍

@MrAlias MrAlias merged commit 859a870 into open-telemetry:main Jul 27, 2023
@MrAlias MrAlias deleted the simp-hist branch July 27, 2023 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@dashpole dashpole dashpole approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@evantorrie evantorrie Awaiting requested review from evantorrie

@XSAM XSAM Awaiting requested review from XSAM XSAM is a code owner

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

@dmathieu dmathieu Awaiting requested review from dmathieu dmathieu is a code owner

Assignees
No one assigned
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
v1.17.0/v0.40.0
Development

Successfully merging this pull request may close these issues.
3 participants
@MrAlias @dashpole @pellared