You've already forked opentelemetry-go
mirror of
https://github.com/open-telemetry/opentelemetry-go.git
synced 2026-06-19 21:45:50 +02:00
Robert Pająk
4ef740f17f
Fixes https://github.com/open-telemetry/opentelemetry-go/issues/7957. - Remove duplicate keys from `attribute.MAP` values by default across SDK resource, log, trace, and metric export paths, including top-level MAP-valued attributes and MAP values nested inside MAP or SLICE values. - Update `log.WithAllowKeyDuplication()` behavior and docs. - Always deduplicate MAP values in resources; `go.opentelemetry.io/otel/sdk/resource` does not provide an opt-out. Resources are typically evaluated once at startup or occasionally updated. The ~12.90% overhead shown in benchmarks will have low impact on an application's runtime latency, and having a strictly normalized Resource map prevents downstream exporter issues. - Preserve top-level `attribute.Set` behavior for resources, scopes, and metric datapoint attributes; this change normalizes MAP-valued attribute values, not the outer attribute collection semantics. - Use last-value-wins semantics and do not increment dropped-attribute counts for MAP duplicate removal. - Keep internal helper code scoped per module: shared source lives in `internal/shared/attrdedup`, then `gotmpl` renders module-local copies under `sdk/internal`, `sdk/log/internal`, and `sdk/metric/internal`. ## Benchmarks Baseline benchmarks were run from a local `main` worktree at `/tmp/opentelemetry-go-main`; branch benchmarks were run from this branch. All comparisons use `-benchmem -count=10`. New helper benchmarks: ```console goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/sdk/internal/attrdedup cpu: 13th Gen Intel(R) Core(TM) i7-13800H │ this.txt │ │ sec/op │ Value/FastPath-20 186.4n ± 19% Value/DuplicateMap-20 363.4n ± 82% Value/NestedMapInSlice-20 607.4n ± 20% geomean 345.2n │ this.txt │ │ B/op │ Value/FastPath-20 0.000 ± 0% Value/DuplicateMap-20 320.0 ± 0% Value/NestedMapInSlice-20 416.0 ± 0% geomean ¹ ¹ summaries must be >0 to compute geomean │ this.txt │ │ allocs/op │ Value/FastPath-20 0.000 ± 0% Value/DuplicateMap-20 2.000 ± 0% Value/NestedMapInSlice-20 4.000 ± 0% geomean ¹ ¹ summaries must be >0 to compute geomean ``` ```console pkg: go.opentelemetry.io/otel/sdk/resource │ main │ branch │ │ sec/op │ sec/op vs base │ MergeResource_1-20 587.9n ± 28% 677.7n ± 17% +15.28% (p=0.043 n=10) MergeResource_2-20 938.5n ± 11% 1011.8n ± 18% ~ (p=0.393 n=10) MergeResource_3-20 1.301µ ± 8% 1.522µ ± 21% +16.95% (p=0.023 n=10) MergeResource_4-20 1.487µ ± 31% 2.006µ ± 12% +34.90% (p=0.000 n=10) MergeResource_6-20 2.409µ ± 12% 2.962µ ± 15% +22.96% (p=0.001 n=10) MergeResource_8-20 3.336µ ± 10% 3.104µ ± 16% ~ (p=0.247 n=10) MergeResource_16-20 7.750µ ± 6% 8.076µ ± 9% ~ (p=0.190 n=10) geomean 1.821µ 2.056µ +12.90% pkg: go.opentelemetry.io/otel/sdk/trace │ main │ branch │ │ sec/op │ sec/op vs base │ RecordingSpanSetAttributes/WithLimit/false-20 7.907µ ± 6% 11.311µ ± 6% +43.06% (p=0.000 n=10) RecordingSpanSetAttributes/WithLimit/true-20 13.29µ ± 15% 17.13µ ± 6% +28.97% (p=0.002 n=10) TraceStart/with_a_simple_span-20 402.1n ± 2% 406.4n ± 2% ~ (p=0.218 n=10) TraceStart/with_several_links-20 521.8n ± 1% 532.4n ± 2% +2.03% (p=0.000 n=10) TraceStart/with_attributes-20 570.5n ± 2% 594.1n ± 2% +4.12% (p=0.000 n=10) TraceStart/ObservabilityEnabled-20 463.5n ± 1% 469.9n ± 1% +1.38% (p=0.003 n=10) SpanSetAttributesOverCapacity-20 2.539µ ± 8% 2.501µ ± 8% ~ (p=0.684 n=10) SpanWithAttributes_4/AlwaysSample-20 1.650µ ± 23% 1.867µ ± 12% ~ (p=0.063 n=10) SpanWithAttributes_4/NeverSample-20 634.5n ± 11% 683.4n ± 19% ~ (p=0.315 n=10) geomean 1.356µ 1.495µ +10.27% ``` Resource and trace allocation counts were unchanged in the same benchstat run. ```console pkg: go.opentelemetry.io/otel/sdk/log │ main │ branch │ │ sec/op │ sec/op vs base │ LoggerProviderLogger-20 505.1n ± 8% 473.9n ± 15% ~ (p=0.123 n=10) SetAttributes/Unique/NoLimits-20 230.9n ± 9% 238.1n ± 7% ~ (p=0.343 n=10) SetAttributes/NestedDeduplication/Enabled-20 836.0n ± 6% 758.1n ± 8% -9.32% (p=0.011 n=10) SetAttributes/NestedDeduplication/Disabled-20 119.7n ± 1% 127.7n ± 8% ~ (p=0.157 n=10) geomean 328.7n 323.3n -1.65% ``` Log allocation counts were unchanged in the same benchstat run. ```console pkg: go.opentelemetry.io/otel/sdk/metric │ main │ branch │ │ sec/op │ sec/op vs base │ MeasureNewAttributeSet/AlwaysOn/Int64Counter-20 725.7n ± 114% 1504.5n ± 15% +107.32% (p=0.043 n=10) MeasureNewAttributeSet/AlwaysOn/Float64Counter-20 1.106µ ± 14% 1.480µ ± 15% +33.82% (p=0.000 n=10) MeasureNewAttributeSet/AlwaysOn/Int64UpDownCounter-20 1.354µ ± 9% 1.334µ ± 13% ~ (p=0.529 n=10) MeasureNewAttributeSet/AlwaysOn/Float64UpDownCounter-20 1.445µ ± 29% 1.439µ ± 14% ~ (p=0.542 n=10) MeasureNewAttributeSet/AlwaysOn/Int64Histogram-20 1.423µ ± 31% 1.453µ ± 29% ~ (p=0.631 n=10) MeasureNewAttributeSet/AlwaysOn/Float64Histogram-20 1.383µ ± 41% 1.416µ ± 20% ~ (p=0.796 n=10) MeasureNewAttributeSet/AlwaysOn/Int64Gauge-20 1.105µ ± 42% 1.527µ ± 8% +38.21% (p=0.023 n=10) MeasureNewAttributeSet/AlwaysOn/Float64Gauge-20 1.598µ ± 10% 1.355µ ± 37% ~ (p=0.247 n=10) MeasureNewAttributeSet/AlwaysOff/Int64Counter-20 739.1n ± 16% 830.0n ± 17% ~ (p=0.280 n=10) MeasureNewAttributeSet/AlwaysOff/Float64Counter-20 299.2n ± 24% 766.7n ± 29% +156.19% (p=0.000 n=10) MeasureNewAttributeSet/AlwaysOff/Int64UpDownCounter-20 322.2n ± 104% 873.2n ± 7% +171.01% (p=0.000 n=10) MeasureNewAttributeSet/AlwaysOff/Float64UpDownCounter-20 740.5n ± 49% 713.8n ± 21% ~ (p=0.481 n=10) MeasureNewAttributeSet/AlwaysOff/Int64Histogram-20 901.1n ± 33% 988.8n ± 12% ~ (p=0.481 n=10) MeasureNewAttributeSet/AlwaysOff/Float64Histogram-20 1.059µ ± 23% 1.009µ ± 21% ~ (p=0.393 n=10) MeasureNewAttributeSet/AlwaysOff/Int64Gauge-20 408.9n ± 50% 788.0n ± 23% +92.71% (p=0.000 n=10) MeasureNewAttributeSet/AlwaysOff/Float64Gauge-20 544.5n ± 37% 662.1n ± 39% ~ (p=0.725 n=10) AsyncMeasureNewAttributeSet/AlwaysOn-20 6.199µ ± 20% 5.766µ ± 26% ~ (p=0.247 n=10) AsyncMeasureNewAttributeSet/TraceBased-20 1.811µ ± 10% 1.806µ ± 11% ~ (p=0.912 n=10) Instrument/instrumentImpl/aggregate-20 2.577µ ± 10% 2.378µ ± 14% ~ (p=0.247 n=10) Instrument/observable/observe-20 1.863µ ± 17% 1.762µ ± 8% ~ (p=0.255 n=10) InstrumentCreation-20 2.346µ ± 17% 2.215µ ± 8% ~ (p=0.315 n=10) geomean 1.102µ 1.324µ +20.12% ``` Metric B/op geomean was +0.10% and allocs/op geomean was unchanged in the same benchstat run. ## Rejected implementations ### Seen-Pool I tested an alternative `deduplicateMapValue` implementation using a pooled `map[string]struct{}` for duplicate detection, modeled after `sdk/log.Record`'s `seenPool`. The candidate was not kept because it regressed all non-opt-out helper benchmark paths. ```console pkg: go.opentelemetry.io/otel/sdk/internal/attrdedup │ baseline │ seenpool │ │ sec/op │ sec/op vs base │ Value/FastPath-20 143.8n ± 0% 201.6n ± 0% +40.15% (p=0.000 n=20) Value/DuplicateMap-20 669.8n ± 9% 862.9n ± 12% +28.83% (p=0.000 n=20) Value/NestedMapInSlice-20 918.2n ± 9% 1115.0n ± 5% +21.43% (p=0.000 n=20) Value/AllowKeyDuplication-20 1.324n ± 0% 1.324n ± 0% ~ (p=0.389 n=20) geomean 104.0n 126.6n +21.68% ``` The candidate did not change allocation counts, but the `sync.Pool` and map operations added measurable CPU overhead. ### Needs-Style Preflight I tested an alternative implementation modeled after `sdk/log.Record.needsValueLimitsOrDedup`: first scan attribute values to determine whether normalization is needed, then normalize only when the scan says a value would change. The refined candidate preserved the opt-out fast path and avoided an extra slice preflight pass, but was not kept because package-level benchmark results were mixed: resource improved, but trace and metric regressed. ```console pkg: go.opentelemetry.io/otel/sdk/internal/attrdedup │ baseline │ candidate │ │ sec/op │ sec/op vs base │ Value/FastPath-20 144.10n ± 0% 92.52n ± 0% -35.79% (p=0.000 n=10) Value/DuplicateMap-20 682.8n ± 19% 677.1n ± 34% ~ (p=0.796 n=10) Value/NestedMapInSlice-20 900.5n ± 7% 975.5n ± 8% +8.33% (p=0.019 n=10) Value/AllowKeyDuplication-20 1.325n ± 0% 1.325n ± 1% ~ (p=0.860 n=10) geomean 104.1n 94.85n -8.88% ``` ```console pkg: go.opentelemetry.io/otel/sdk/resource MergeResource geomean: 2.209µ -> 1.896µ (-14.17%) pkg: go.opentelemetry.io/otel/sdk/trace Trace benchmark geomean: 1.474µ -> 1.576µ (+6.90%) ``` ```console pkg: go.opentelemetry.io/otel/sdk/log Log benchmark geomean: 330.0n -> 330.6n (+0.18%) ``` ```console pkg: go.opentelemetry.io/otel/sdk/metric Metric benchmark geomean: 1.355µ -> 1.455µ (+7.38%) ``` All candidate allocation geomeans were unchanged. Because the candidate did not improve performance consistently across affected export paths, this PR keeps the original lazy allocation-free no-op implementation.