Part of https://github.com/open-telemetry/opentelemetry-go/issues/8110
Related to
https://github.com/open-telemetry/opentelemetry-go/issues/5882.
I'm hoping to find a better way to support experimental Options types in
our API packages. This is one approach to consider.
This contains no public API changes. It introduces a type:
`ExperimentalOption` in `/metric/internal/x`, which can be used by our
experimental options defined outside of the module. Options that embed
this interface are ignored by `New*Config` builder functions in the
metrics API to prevent them from panicing when used. Only SDKs that
explicitly support the experimental option in question will respect it.
Alternative SDKs will ignore the experimental options. We would still
need to treat ExperimentalOption as a stable artifact, since the SDK
will indirectly depend on it.
See
https://github.com/open-telemetry/opentelemetry-go/compare/main...dashpole:opentelemetry-go:attributes_advisory
for how this would be used to support the advisory attributes parameter.
Per
https://github.com/open-telemetry/opentelemetry-go/pull/7266#issuecomment-3237027300
Related to
https://github.com/open-telemetry/opentelemetry-go/issues/7217
## What
This PR adds `WithInstrumentationAttributeSet` option functions to the
`log`, `metric`, and `trace` packages as suggested in
https://github.com/open-telemetry/opentelemetry-go/pull/7266#issuecomment-3237027300.
These new functions provide a more concurrent-safe alternative to the
existing `WithInstrumentationAttributes` functions by accepting a
pre-constructed `attribute.Set` instead of variadic `attribute.KeyValue`
parameters.
## Why
As discussed in #7266, the existing `WithInstrumentationAttributes`
functions can lead to data races when used concurrently because
`attribute.NewSet()` may mutate the passed slice in-place. While the
issue was partially addressed by moving the `attribute.NewSet()` call
outside the closure, the best long-term solution is to provide an
alternative that accepts an immutable `attribute.Set`.
**Benefits:**
1. **Concurrent Safety**: Since `attribute.Set` is immutable, these
functions are inherently safe for concurrent use
2. **Performance**: Avoids repeated calls to `attribute.NewSet()` when
the same attributes are used multiple times
3. **Consistency**: Matches the existing pattern used in
`metric.WithAttributeSet()`
4. **Flexibility**: Allows users to pre-compute attribute sets and reuse
them
Deprecating `WithInstrumentationAttributes` is out of scope. See
https://github.com/open-telemetry/opentelemetry-go/pull/7287#issuecomment-3245820459.
## Benchmarks
```
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/trace
cpu: 13th Gen Intel(R) Core(TM) i7-13800H
BenchmarkNewTracerConfig/with_no_options-20 280298306 4.268 ns/op 0 B/op 0 allocs/op
BenchmarkNewTracerConfig/with_an_instrumentation_version-20 33389427 30.84 ns/op 0 B/op 0 allocs/op
BenchmarkNewTracerConfig/with_a_schema_url-20 35441077 30.46 ns/op 0 B/op 0 allocs/op
BenchmarkNewTracerConfig/with_instrumentation_attribute-20 17607649 88.23 ns/op 64 B/op 1 allocs/op
BenchmarkNewTracerConfig/with_instrumentation_attribute_set-20 38336211 31.30 ns/op 0 B/op 0 allocs/op
```
```
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/metric
cpu: 13th Gen Intel(R) Core(TM) i7-13800H
BenchmarkNewMeterConfig/with_no_options-20 262998199 4.525 ns/op 0 B/op 0 allocs/op
BenchmarkNewMeterConfig/with_an_instrumentation_version-20 40483780 29.31 ns/op 0 B/op 0 allocs/op
BenchmarkNewMeterConfig/with_a_schema_url-20 39162420 30.58 ns/op 0 B/op 0 allocs/op
BenchmarkNewMeterConfig/with_instrumentation_attribute-20 19900275 77.50 ns/op 64 B/op 1 allocs/op
BenchmarkNewMeterConfig/with_instrumentation_attribute_set-20 37519020 31.93 ns/op 0 B/op 0 allocs/op
```
```
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/log
cpu: 13th Gen Intel(R) Core(TM) i7-13800H
BenchmarkNewLoggerConfig/with_no_options-20 271100760 4.322 ns/op 0 B/op 0 allocs/op
BenchmarkNewLoggerConfig/with_an_instrumentation_version-20 38392390 30.77 ns/op 0 B/op 0 allocs/op
BenchmarkNewLoggerConfig/with_a_schema_url-20 39615074 30.25 ns/op 0 B/op 0 allocs/op
BenchmarkNewLoggerConfig/with_instrumentation_attribute-20 17108463 82.51 ns/op 64 B/op 1 allocs/op
BenchmarkNewLoggerConfig/with_instrumentation_attribute_set-20 37746534 31.70 ns/op 0 B/op 0 allocs/op
```
* Accept scope attr during Meter creation
* Fix lint
* Add changes to changelog
* Return a Set from InstrumentationAttributes
Likely these attributes will be stored as a Set in the SDK. Don't cause
two conversions because we return a slice here.
* Add config tests
* Fix lint
* Add WithScopeAttributes MeterOption to metric pkg
* Add MeterConfig unit tests
* Add changes to changelog
* Fix import linting
* Update MeterProvider documentation
Include information about how to use WithScopeAttributes.
* Empty All of the metrics dir
* Add instrument api with documentation
* Add a NoOp implementation.
* Updated to the new config standard
* Address PR comments
* This change moves components to sdk/metrics
The Moved components are:
- metric/metrictest
- metric/number
- metric/internal/registry
- metric/sdkapi
* The SDK changes necessary to satasify the new api
* This fixes the remaing tests.
* Update changelog
* refactor the Noop meter and instruments into one package.
* Renamed pkg.Instruments to pkg.InstrumentProvider
Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com>
* Change trace options to accept type not pointer
Add benchmark to show allocation improvement.
* Update CONTRIBUTING.md guidelines
* Update all Option iface
* Fix grammar in CONTRIBUTING
* factor instrumentation library out of the instrument descriptor
* SDK tests pass
* checkpoint work
* otlp and opencensus tests passing
* prometheus
* tests pass, working on lint
* lint applied: MetricReader->Reader
* comments
* Changelog
* Apply suggestions from code review
Co-authored-by: alrex <alrex.boten@gmail.com>
* remove an interdependency
* fix build
* re-indent one
* Apply suggestions from code review
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
* Lint&feedback
* update after rename
* comment fix
* style fix for meter options
* remove libraryReader, let Controller implement the reader API directly
* rename 'impl' field to 'provider'
* remove a type assertion
* move metric/registry into internal; move registry.MeterProvider into metric controller
* add test for controller registry function
* CheckpointSet->Reader everywhere
* lint
* remove two unnecessary accessor methods; Controller implements MeterProvider and InstrumentationLibraryReader directly, no need to get these
* use a sync.Map
* ensure the initOnce is always called; handle multiple errors
* Apply suggestions from code review
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
* cleanup locking in metrictest
* Revert "ensure the initOnce is always called; handle multiple errors"
This reverts commit 3356eb5ed0.
* Revert "use a sync.Map"
This reverts commit ea7bc599bd.
* restore the TODO about sync.Map
Co-authored-by: alrex <alrex.boten@gmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
David Ashpole
Andrew Wilkins
Robert Pająk
Tyler Yahn
Aaron Clawson
Aaron Clawson
Bogdan Drutu
Joshua MacDonald
Krzesimir Nowak