Add cardinality limiting to the metric SDK as an experimental feature by MrAlias · Pull Request #4457 · open-telemetry/opentelemetry-go

MrAlias · 2023-08-19T00:02:33Z

Part of #4095

codecov · 2023-08-19T00:06:29Z

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

@@          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%)`	⬆️

The Attribute method is still inlinable.

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.

MadVikingGod · 2023-12-14T22:22:48Z

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 · 2023-12-15T16:33:46Z

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 · 2023-12-15T16:33:50Z

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 · 2023-12-15T16:36:12Z

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 added 5 commits August 18, 2023 16:37

Add agg limiting func

3bc9dcc

Add unit test for limitAttr

c8ce214

Add limiting to aggregate types

c79f14e

Add internal x pkg for experimental feature-flagging

6aa1640

Connect cardinality limit to metric SDK

cc11cb0

dmathieu mentioned this pull request Sep 12, 2023

Remove high cardanility metrics from otelhttp open-telemetry/opentelemetry-go-contrib#4277

Merged

MadVikingGod reviewed Sep 29, 2023

View reviewed changes

Comment thread sdk/metric/pipeline.go Outdated

MadVikingGod reviewed Sep 29, 2023

View reviewed changes

Comment thread sdk/metric/internal/x/x.go Outdated

MrAlias mentioned this pull request Nov 13, 2023

Add the internal experimental metric feature package #4715

Merged

This comment was marked as outdated.

Sign in to view

MrAlias added 11 commits December 6, 2023 11:02

Merge branch 'main' into cardinality-limit-x

07dc9b9

Replace limitAttr fn with limiter type

62311eb

The Attribute method is still inlinable.

Use x.CardinalityLimit directly

b53c71c

Simplify limiter test

92efe65

Add limiter benchmark

27df16c

Document the AggregationLimit field

c2d5246

Test sum limits

426b29d

Test limit for last value

3a59317

Test histogram limit

0354e68

Test the ExponentialHistogram limit

4874fcb

This comment was marked as outdated.

Sign in to view

MrAlias mentioned this pull request Dec 6, 2023

Refactor exponential histogram tests to use existing fixtures #4747

Merged

MrAlias added 3 commits December 6, 2023 14:23

Fix lint

3fa01f1

Add docs

ea4fe06

Merge branch 'main' into cardinality-limit-x

b022999

MrAlias marked this pull request as ready for review December 9, 2023 16:28

MrAlias requested review from Aneurysm9 and evantorrie as code owners December 9, 2023 16:28

MrAlias requested review from XSAM, dashpole, dmathieu, hanyuancheung and pellared as code owners December 9, 2023 16:28

dashpole approved these changes Dec 11, 2023

View reviewed changes

Comment thread sdk/metric/internal/aggregate/limit.go Outdated

MrAlias and others added 2 commits December 12, 2023 08:06

Rename aggregation field to aggLimit

5bc739e

Merge branch 'main' into cardinality-limit-x

320fee0

Merge branch 'main' into cardinality-limit-x

a8fbbf7

pellared approved these changes Dec 19, 2023

View reviewed changes

Merge branch 'main' into cardinality-limit-x

41f950b

MrAlias merged commit e3bf787 into open-telemetry:main Dec 19, 2023

MrAlias deleted the cardinality-limit-x branch December 19, 2023 15:53

MrAlias mentioned this pull request Dec 19, 2023

Investigate if attribute filtering should be in the instrument or aggregator #3011

Open

MrAlias added this to the v1.22.0 milestone Jan 11, 2024

MadVikingGod mentioned this pull request Jan 11, 2024

Release v1.22.0/v0.45.0 #4821

Merged

MrAlias mentioned this pull request Jan 18, 2024

Add cardinality limits to metrics pipelines #4095

Closed

utpilla mentioned this pull request Feb 26, 2024

Stabilize Overflow attribute section under Cardinality Limits open-telemetry/opentelemetry-specification#3904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Add cardinality limiting to the metric SDK as an experimental feature#4457
MrAlias merged 23 commits into
open-telemetry:mainfrom
MrAlias:cardinality-limit-x

MrAlias commented Aug 19, 2023 •

edited

Loading

Uh oh!

codecov Bot commented Aug 19, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

MadVikingGod commented Dec 14, 2023

Uh oh!

MrAlias commented Dec 15, 2023

Uh oh!

MrAlias commented Dec 15, 2023 •

edited

Loading

Uh oh!

MrAlias commented Dec 15, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

MrAlias commented Aug 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

MadVikingGod commented Dec 14, 2023

Uh oh!

MrAlias commented Dec 15, 2023

Uh oh!

MrAlias commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrAlias commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MrAlias commented Aug 19, 2023 •

edited

Loading

codecov Bot commented Aug 19, 2023 •

edited

Loading

MrAlias commented Dec 15, 2023 •

edited

Loading

MrAlias commented Dec 15, 2023 •

edited

Loading