[Proposal] Accept a Set for metric measurement operations

[MrAlias]

Resolves #3943

• Prevents data races when a user passes the same attribute slice to multiple measurement operations concurrently. All measurement methods are required by the specification to concurrent safe¹.
• Allows users to allocate a single attribute.Set for all measurement methods instead of having each method create its own attribute.Set for the same data.

Footnotes

1. https://github.com/open-telemetry/opentelemetry-specification/blob/9352c2524ac03b31f4b845323b351d32ab3adb43/specification/metrics/api.md#concurrency-requirements ↩

[codecov]

Codecov Report

Merging #3947 (d562adf) into main (65ebe5e) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #3947   +/-   ##
=====================================
  Coverage   81.7%   81.7%           
=====================================
  Files        171     171           
  Lines      12951   12949    -2     
=====================================
  Hits       10592   10592           
+ Misses      2136    2134    -2     
  Partials     223     223           

Impacted Files	Coverage Δ
metric/instrument/asyncfloat64.go	100.0% <ø> (ø)
metric/instrument/asyncint64.go	100.0% <ø> (ø)
metric/instrument/syncfloat64.go	100.0% <ø> (ø)
metric/instrument/syncint64.go	100.0% <ø> (ø)
internal/global/instruments.go	60.8% <100.0%> (ø)
metric/noop/noop.go	100.0% <100.0%> (ø)
sdk/metric/instrument.go	95.9% <100.0%> (-0.2%)	⬇️
sdk/metric/meter.go	89.5% <100.0%> (ø)

... and 1 file with indirect coverage changes

[MrAlias]

Benchmark of counter Add

• old.txt: f4a9d78
• new.txt: 3425a1a

$ benchstat old.txt new.txt
name                                old time/op    new time/op    delta
CounterAddNoAttrs-8                   42.0ns ± 6%    42.2ns ± 6%      ~     (p=0.529 n=10+10)
CounterAddOneAttr-8                    258ns ± 8%     104ns ± 6%   -59.66%  (p=0.000 n=10+10)
CounterAddOneInvalidAttr-8             361ns ± 9%     172ns ± 7%   -52.45%  (p=0.000 n=10+10)
CounterAddSingleUseAttrs-8             851ns ± 1%     843ns ± 3%      ~     (p=0.156 n=9+10)
CounterAddSingleUseInvalidAttrs-8     1.24µs ± 3%    1.22µs ± 1%    -1.67%  (p=0.008 n=10+10)
CounterAddSingleUseFilteredAttrs-8    1.38µs ± 3%    1.36µs ± 1%    -1.37%  (p=0.011 n=10+10)
CounterCollectOneAttr-8                300ns ± 8%     145ns ± 2%   -51.76%  (p=0.000 n=10+10)
CounterCollectTenAttrs-8              2.87µs ± 6%    2.86µs ±10%      ~     (p=0.617 n=10+10)

name                                old alloc/op   new alloc/op   delta
CounterAddNoAttrs-8                    0.00B          0.00B           ~     (all equal)
CounterAddOneAttr-8                     128B ± 0%        0B       -100.00%  (p=0.000 n=10+10)
CounterAddOneInvalidAttr-8              256B ± 0%        0B       -100.00%  (p=0.000 n=10+10)
CounterAddSingleUseAttrs-8              226B ± 0%      225B ± 1%      ~     (p=0.054 n=8+9)
CounterAddSingleUseInvalidAttrs-8       379B ± 0%      379B ± 0%      ~     (all equal)
CounterAddSingleUseFilteredAttrs-8      571B ± 0%      571B ± 0%      ~     (all equal)
CounterCollectOneAttr-8                 144B ± 0%       16B ± 0%   -88.89%  (p=0.000 n=10+10)
CounterCollectTenAttrs-8              1.30kB ± 0%    1.30kB ± 0%      ~     (all equal)

name                                old allocs/op  new allocs/op  delta
CounterAddNoAttrs-8                     0.00           0.00           ~     (all equal)
CounterAddOneAttr-8                     2.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)
CounterAddOneInvalidAttr-8              2.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)
CounterAddSingleUseAttrs-8              2.00 ± 0%      2.00 ± 0%      ~     (all equal)
CounterAddSingleUseInvalidAttrs-8       2.00 ± 0%      2.00 ± 0%      ~     (all equal)
CounterAddSingleUseFilteredAttrs-8      4.00 ± 0%      4.00 ± 0%      ~     (all equal)
CounterCollectOneAttr-8                 3.00 ± 0%      1.00 ± 0%   -66.67%  (p=0.000 n=10+10)
CounterCollectTenAttrs-8                21.0 ± 0%      21.0 ± 0%      ~     (all equal)

[merlimat]

@MrAlias Nice comparison! It would be great to also show the difference when one can keep reusing the attributes set.

// Old API
func BenchmarkCounterAddMultiUseAttrs(b *testing.B) {
	ctx, _, cntr := benchCounter(b)

       attrs := []attribute.KeyValue{attribute.Int("K", i)}

	for i := 0; i < b.N; i++ {
		cntr.Add(ctx, 1, attrs...)
	}
}

// New API
func BenchmarkCounterAddMultiUseAttrs(b *testing.B) {
	ctx, _, cntr := benchCounter(b)

       attrs := attribute.NewSet(attribute.Int("K", i))

	for i := 0; i < b.N; i++ {
		cntr.Add(ctx, 1, attrs)
	}
}

[MrAlias]

@MrAlias Nice comparison! It would be great to also show the difference when one can keep reusing the attributes set.

// Old API
func BenchmarkCounterAddMultiUseAttrs(b *testing.B) {
	ctx, _, cntr := benchCounter(b)

       attrs := []attribute.KeyValue{attribute.Int("K", i)}

	for i := 0; i < b.N; i++ {
		cntr.Add(ctx, 1, attrs...)
	}
}

// New API
func BenchmarkCounterAddMultiUseAttrs(b *testing.B) {
	ctx, _, cntr := benchCounter(b)

       attrs := attribute.NewSet(attribute.Int("K", i))

	for i := 0; i < b.N; i++ {
		cntr.Add(ctx, 1, attrs)
	}
}

@merlimat what you describe is the CounterAddOneAttr benchmark shown above:

https://github.com/open-telemetry/opentelemetry-go/pull/3947/files#diff-8c2bfbe3dd660c478bb807eb4d1922321893af4242bda90c3ed12e2a716c28e6R45-R52

[merlimat]

thanks! My bad, I couldn't find the updated benchmark code in the PR.

[MadVikingGod]

Why are some of the Sets created within the loop and some not?

[MrAlias]

Some of them depend on the loop variable and others do not.

[MadVikingGod]

Prevents data races when a user passes the same attribute slice to multiple measurement operations concurrently.

This doesn't solve the actual underlying cause (NetSet has a datarace in it), but I think that can be solved in a different issue.

[pellared]

After few days of digesting the design and my thoughts, I support this change. I also left a comment in the issue.

[MrAlias]

Closing in favor of #3971