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

Add cardinality limiting to the metric SDK as an experimental feature #4457

Merged
merged 23 commits into from Dec 19, 2023
Merged

Add cardinality limiting to the metric SDK as an experimental feature #4457

merged 23 commits into from Dec 19, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Aug 19, 2023 •
edited

Part of #4095

@codecov
Copy link

codecov bot commented Aug 19, 2023 •
edited

Codecov Report

Merging #4457 (41f950b) into main (cb8cb2d) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4457   +/-   ##
=====================================
  Coverage   82.2%   82.2%           
=====================================
  Files        225     226    +1     
  Lines      18315   18342   +27     
=====================================
+ Hits       15058   15085   +27     
  Misses      2975    2975           
  Partials     282     282
Files Coverage Δ
sdk/metric/internal/aggregate/aggregate.go 100.0% <100.0%> (ø)
...metric/internal/aggregate/exponential_histogram.go 96.2% <100.0%> (+<0.1%) ⬆️
sdk/metric/internal/aggregate/histogram.go 100.0% <100.0%> (ø)
sdk/metric/internal/aggregate/lastvalue.go 100.0% <100.0%> (ø)
sdk/metric/internal/aggregate/limit.go 100.0% <100.0%> (ø)
sdk/metric/internal/aggregate/sum.go 100.0% <100.0%> (ø)
sdk/metric/internal/x/x.go 100.0% <ø> (ø)
sdk/metric/pipeline.go 86.4% <100.0%> (+0.2%) ⬆️

@dmathieu dmathieu mentioned this pull request Sep 12, 2023
Merged
@MrAlias MrAlias mentioned this pull request Nov 13, 2023
Merged
@MrAlias

This comment was marked as outdated.

Sign in to view
@MrAlias
Merge branch 'main' into cardinality-limit-x
07dc9b9
@MrAlias
Replace limitAttr fn with limiter type
62311eb 
The Attribute method is still inlinable.
@MrAlias
Use x.CardinalityLimit directly
b53c71c
@MrAlias
Simplify limiter test
92efe65
@MrAlias
Add limiter benchmark
27df16c
@MrAlias
Document the AggregationLimit field
c2d5246
@MrAlias
Test sum limits
426b29d
@MrAlias
Test limit for last value
3a59317
@MrAlias
Test histogram limit
0354e68
@MrAlias
Refactor expo hist test to use existing fixtures
f1f3be7 
The tests for the exponential histogram create their own testing
fixtures. There is nothing these new fixtures do that cannot already be
done with the existing testing fixtures used by all the other aggregate
functions. Unify the exponential histogram testing to use the existing
fixtures.
@MrAlias
Test the ExponentialHistogram limit
4874fcb
@MrAlias

This comment was marked as outdated.

Sign in to view
@MrAlias MrAlias mentioned this pull request Dec 6, 2023
Merged
@MrAlias MrAlias marked this pull request as ready for review December 9, 2023 16:28
dashpole
dashpole approved these changes Dec 11, 2023
View reviewed changes
sdk/metric/internal/aggregate/limit.go Outdated Show resolved Hide resolved
@MadVikingGod
Copy link
Contributor

I'm concerned about the performance of this patch, as it potentially adds another map lookup in the hot path.

Have you checked that it doesn't have an impact on the disabled path at least, and what the effect might be on the eventual default config (e.g., large enough that all attributes will fit)

Other than that, I think this will work.

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 15, 2023

I'm concerned about the performance of this patch, as it potentially adds another map lookup in the hot path.

Given a map lookup is on the order of 10s of nanoseconds I'm not sure the overhead of this operation is going to be too impactful. I'm also not sure how to implement this feature in a more performant way. At some level we will need to determining from some saved memory location if the attributes have been seen. Did you have an alternate proposal?

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 15, 2023 •
edited

Have you checked that it doesn't have an impact on the disabled path at least

Given this is gated with a conditional the expected performance impact is a slight. With branch prediction being a thing, I imagine this is going to be negligible.

Targeted benchmarking have validated this assumption, i.e.

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               │
ExponentialHistogram/Int64/Cumulative/100/Measure-8     15.47µ ± 4%   14.71µ ± 2%  -4.93% (p=0.000 n=10)
ExponentialHistogram/Int64/Delta/100/Measure-8          15.28µ ± 5%   15.38µ ± 3%       ~ (p=0.739 n=10)
ExponentialHistogram/Float64/Cumulative/100/Measure-8   15.80µ ± 3%   14.85µ ± 4%  -6.00% (p=0.002 n=10)
ExponentialHistogram/Float64/Delta/100/Measure-8        16.03µ ± 5%   14.90µ ± 2%  -7.05% (p=0.000 n=10)
Histogram/Int64/Cumulative/100/Measure-8                12.54µ ± 3%   12.10µ ± 4%  -3.49% (p=0.029 n=10)
Histogram/Int64/Delta/100/Measure-8                     12.80µ ± 4%   12.37µ ± 3%       ~ (p=0.063 n=10)
Histogram/Float64/Cumulative/100/Measure-8              12.45µ ± 5%   11.97µ ± 3%  -3.82% (p=0.015 n=10)
Histogram/Float64/Delta/100/Measure-8                   12.04µ ± 3%   12.05µ ± 3%       ~ (p=0.912 n=10)
Sum/Int64/Cumulative/100/Measure-8                      10.62µ ± 5%   10.46µ ± 5%       ~ (p=0.481 n=10)
Sum/Int64/Delta/100/Measure-8                           10.62µ ± 3%   10.87µ ± 3%       ~ (p=0.075 n=10)
Sum/Float64/Cumulative/100/Measure-8                    10.81µ ± 2%   10.93µ ± 4%       ~ (p=0.436 n=10)
Sum/Float64/Delta/100/Measure-8                         10.56µ ± 6%   10.67µ ± 3%       ~ (p=0.853 n=10)
geomean                                                 12.75µ        12.48µ       -2.12%

                                                      │   old.txt    │               new.txt               │
                                                      │     B/op     │    B/op     vs base                 │
ExponentialHistogram/Int64/Cumulative/100/Measure-8     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Int64/Delta/100/Measure-8          0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Cumulative/100/Measure-8   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Delta/100/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/100/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/100/Measure-8                   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Cumulative/100/Measure-8                      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Delta/100/Measure-8                           0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Cumulative/100/Measure-8                    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/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                 │
ExponentialHistogram/Int64/Cumulative/100/Measure-8     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Int64/Delta/100/Measure-8          0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Cumulative/100/Measure-8   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Delta/100/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/100/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/100/Measure-8                   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Cumulative/100/Measure-8                      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Delta/100/Measure-8                           0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Cumulative/100/Measure-8                    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/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

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 15, 2023 •
edited

what the effect might be on the eventual default config (e.g., large enough that all attributes will fit)

The default is defined to be 2000: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#configuration-1

I'm not aware of any objective data validating this number, but it is something we will adopt from the specification.

The current PR does not use this default. The current default is unlimited as the feature is disabled by default.

@MrAlias MrAlias merged commit e3bf787 into open-telemetry:main Dec 19, 2023
25 checks passed
@MrAlias MrAlias deleted the cardinality-limit-x branch December 19, 2023 15:53
@MrAlias MrAlias mentioned this pull request Dec 19, 2023
Open
@MrAlias MrAlias added this to the v1.22.0 milestone Jan 11, 2024
@MadVikingGod MadVikingGod mentioned this pull request Jan 11, 2024
Merged
@MrAlias MrAlias mentioned this pull request Jan 18, 2024
Closed
@utpilla utpilla mentioned this pull request Feb 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashpole dashpole dashpole approved these changes

@MadVikingGod MadVikingGod MadVikingGod left review comments

@pellared pellared pellared approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

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

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

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.22.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MrAlias @MadVikingGod @dashpole @pellared