Stop duplicating metrics attrSet

[carsonip]

Insignificant gains, but doesn't hurt to fix.

benchstat:

goos: linux
goarch: amd64
pkg: github.com/elastic/apm-aggregation/aggregators
cpu: 11th Gen Intel(R) Core(TM) i7-11800H @ 2.30GHz
                            │ bench-out/metrics-attr-set-before │  bench-out/metrics-attr-set-after  │
                            │              sec/op               │   sec/op     vs base               │
AggregateCombinedMetrics-16                         3.949µ ± 1%   4.013µ ± 2%  +1.63% (p=0.009 n=10)
AggregateBatchSerial-16                             10.13µ ± 2%   10.25µ ± 4%       ~ (p=0.896 n=10)
AggregateBatchParallel-16                           10.63µ ± 7%   10.67µ ± 4%       ~ (p=0.912 n=10)
CombinedMetricsEncoding-16                          4.934µ ± 4%   5.005µ ± 3%       ~ (p=1.000 n=10)
CombinedMetricsDecoding-16                          4.465µ ± 1%   4.487µ ± 2%       ~ (p=0.684 n=10)
CombinedMetricsToBatch-16                           412.3µ ± 2%   415.1µ ± 3%       ~ (p=0.280 n=10)
EventToCombinedMetrics-16                           813.0n ± 3%   815.7n ± 2%       ~ (p=0.896 n=10)
geomean                                             8.474µ        8.549µ       +0.88%

                            │ bench-out/metrics-attr-set-before │   bench-out/metrics-attr-set-after    │
                            │               B/op                │     B/op      vs base                 │
AggregateCombinedMetrics-16                      4.846Ki ± 2%     4.854Ki ± 1%       ~ (p=0.565 n=10)
AggregateBatchSerial-16                          11.29Ki ± 1%     11.27Ki ± 0%       ~ (p=0.739 n=10)
AggregateBatchParallel-16                        11.26Ki ± 1%     11.24Ki ± 1%       ~ (p=0.838 n=10)
CombinedMetricsEncoding-16                         0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
CombinedMetricsDecoding-16                       10.01Ki ± 0%     10.01Ki ± 0%       ~ (p=0.907 n=10)
CombinedMetricsToBatch-16                        37.66Ki ± 0%     37.66Ki ± 0%       ~ (p=1.000 n=10) ¹
EventToCombinedMetrics-16                          0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                       ²                 -0.02%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                            │ bench-out/metrics-attr-set-before │  bench-out/metrics-attr-set-after   │
                            │             allocs/op             │ allocs/op   vs base                 │
AggregateCombinedMetrics-16                        39.00 ± 3%     38.00 ± 3%  -2.56% (p=0.001 n=10)
AggregateBatchSerial-16                            61.00 ± 2%     60.00 ± 2%  -1.64% (p=0.005 n=10)
AggregateBatchParallel-16                          61.00 ± 0%     60.00 ± 0%  -1.64% (p=0.000 n=10)
CombinedMetricsEncoding-16                         0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
CombinedMetricsDecoding-16                         40.00 ± 0%     40.00 ± 0%       ~ (p=1.000 n=10) ¹
CombinedMetricsToBatch-16                          308.0 ± 0%     308.0 ± 0%       ~ (p=1.000 n=10) ¹
EventToCombinedMetrics-16                          0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                       ²               -0.84%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

[dmathieu]

NewSet seems to add an additional allocation (per https://pkg.go.dev/go.opentelemetry.io/otel/attribute#NewSet). Should we use NewSetWithSortable and a nil sortable instead?

[carsonip]

Good call, I'll look into it.

[carsonip]

The alloc before this PR was due to metric.attrOpt escaping:

aggregators/writer.go:133:40: ... argument escapes to heap
aggregators/writer.go:133:72: metric.attrOpt{...} escapes to heap

A nil sortable doesn't work. And if we pass in a sortable to NewSetWithSortable, we'll have to allocate it ourselves anyway. So we might as well use the otel pooled sortable.

[lahsivjar]

I think this has been addressed in this PR, @carsonip can we confirm and close this PR if it is not needed?

[carsonip]

can we confirm and close this PR if it is not needed?

Yes, thanks. Closing.