trace,metric,log: change WithInstrumentationAttributes to not de-depuplicate the passed attributes in a closure #7266
Conversation
WithInstrumentationAttributes is creating a closure with a reference to a slice which is later passed to attribute.NewSet. attribute.NewSet may mutate the slice, so this will lead to a data race when the option is applied concurrently. We can fix this by moving the call to attribute.NewSet outside the closure.
axw
force-pushed
the
clone-attribute-set
branch
from
062026e to
2557df6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7266 +/- ##
=======================================
- Coverage 83.0% 82.9% -0.1%
=======================================
Files 267 267
Lines 25012 24982 -30
=======================================
- Hits 20760 20728 -32
- Misses 3876 3878 +2
Partials 376 376
🚀 New features to boost your workflow:
|
axw
requested review from
MrAlias,
XSAM,
dashpole,
dmathieu,
flc1125 and
pellared
as code owners
MrAlias
left a comment
MrAlias
left a comment
There was a problem hiding this comment.
I'm skeptical this is a needed change. The options and function options are not defined as concurrent safe. There should be no user expectation that they are. The filed bug is using them assuming they are.
Additionally, this is only making them partially concurrent safe, there are still data-races a user can cause by calling these with shared attributes.
|
Removing the lazy-evaluation of the option function seems like something we should include. If a user is creating the option there is little chance they will not be using it in normal use-case. There is no reason to delay creating the set. Additionally, it prevents bugs like this: func TestNewLoggerConfig(t *testing.T) {
version := "v1.1.1"
schemaURL := "https://opentelemetry.io/schemas/1.0.0"
attr := []attribute.KeyValue{
attribute.String("user", "alice"),
attribute.Bool("admin", true),
}
options := []log.LoggerOption{
log.WithInstrumentationVersion(version),
log.WithSchemaURL(schemaURL),
log.WithInstrumentationAttributes(attr...),
}
set := attribute.NewSet(attr...)
// Modifications to attr should not affect the config.
attr[0] = attribute.String("user", "bob")
c := log.NewLoggerConfig(options...)
assert.Equal(t, version, c.InstrumentationVersion(), "instrumentation version")
assert.Equal(t, schemaURL, c.SchemaURL(), "schema URL")
assert.Equal(t, set, c.InstrumentationAttributes(), "instrumentation attributes")
}However, I do not think we should provide any concurrency grantees around this function or type similar to what we have already done. |
The current modification seems to solve this problem. |
There was a problem hiding this comment.
While I have nothing against the changes in code (comptung the set before the closure), I do agree with @MrAlias that it does not prevent race conditions (the changelog entry is not true). But I like the change because attribute.NewSet(attr...) is called once instead of being called for each logger. So it is more of an optimization (per-comuptation). I would accept such PR if the changelog entry is updated and there is a benchmark that showcases the optimization.
Co-authored-by: Robert Pająk <[email protected]>
@pellared it does solve a data race, but it doesn't solve all data races -- see #7266 (comment). Importantly, it ensures that If we wanted to prevent all races, then |
|
BTW, in case it was missed in #7217: this race affects opentelemetry-collector. The collector has the race because it's using otelzap, which creates the option once here: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/7ac87bd923d177b571dbb24b6497a4f69e1a6aff/bridges/otelzap/core.go#L142-L144 ... and then multiple Loggers may be created on-demand:
An alternative to this change would be to:
|
+1 |
pellared
changed the title
Per #7266 (comment) Related to #7217 ## What This PR adds `WithInstrumentationAttributeSet` option functions to the `log`, `metric`, and `trace` packages as suggested in #7266 (comment). 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 #7287 (comment). ## 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 ```
Done. Of course, it depends on how you construct the option - the benchmarks create the option outside of the hot loop, hence they show an improvement. |
There was a problem hiding this comment.
This PR is still aspirationally trying to impose concurrent safety guarantees for WithInstrumentationAttributes and the InstrumentationOption it returns. This is wrong, we do not, nor should, provide such guarantees.
Adjusting the lazy evaluation of the set makes sense as it is likely the attribute slice passed to WithInstrumentationAttributes will be reused and cause unexpected behavior. However, implying these functions and types are safe for concurrent use is an overstatement and should be removed.
pellared
changed the title
Co-authored-by: Robert Pająk <[email protected]> Co-authored-by: Flc゛ <[email protected]>
Co-authored-by: Flc゛ <[email protected]>
This prevents races while creating the set.
## Overview ### Added - Greatly reduce the cost of recording metrics in `go.opentelemetry.io/otel/sdk/metric` using hashing for map keys. (#7175) - Add `WithInstrumentationAttributeSet` option to `go.opentelemetry.io/otel/log`, `go.opentelemetry.io/otel/metric`, and `go.opentelemetry.io/otel/trace` packages. This provides a concurrent-safe and performant alternative to `WithInstrumentationAttributes` by accepting a pre-constructed `attribute.Set`. (#7287) - Add experimental observability for the Prometheus exporter in `go.opentelemetry.io/otel/exporters/prometheus`. Check the `go.opentelemetry.io/otel/exporters/prometheus/internal/x` package documentation for more information. (#7345) - Add experimental observability metrics in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`. (#7353) - Add temporality selector functions `DeltaTemporalitySelector`, `CumulativeTemporalitySelector`, `LowMemoryTemporalitySelector` to `go.opentelemetry.io/otel/sdk/metric`. (#7434) - Add experimental observability metrics for simple log processor in `go.opentelemetry.io/otel/sdk/log`. (#7548) - Add experimental observability metrics in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`. (#7459) - Add experimental observability metrics in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`. (#7486) - Add experimental observability metrics for simple span processor in `go.opentelemetry.io/otel/sdk/trace`. (#7374) - Add experimental observability metrics in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#7512) - Add experimental observability metrics for manual reader in `go.opentelemetry.io/otel/sdk/metric`. (#7524) - Add experimental observability metrics for periodic reader in `go.opentelemetry.io/otel/sdk/metric`. (#7571) - Support `OTEL_EXPORTER_OTLP_LOGS_INSECURE` and `OTEL_EXPORTER_OTLP_INSECURE` environmental variables in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#7608) - Add `Enabled` method to the `Processor` interface in `go.opentelemetry.io/otel/sdk/log`. All `Processor` implementations now include an `Enabled` method. (#7639) - The `go.opentelemetry.io/otel/semconv/v1.38.0` package. The package contains semantic conventions from the `v1.38.0` version of the OpenTelemetry Semantic Conventions. See the [migration documentation](./semconv/v1.38.0/MIGRATION.md) for information on how to upgrade from `go.opentelemetry.io/otel/semconv/v1.37.0.`(#7648) ### Changed - `Distinct` in `go.opentelemetry.io/otel/attribute` is no longer guaranteed to uniquely identify an attribute set. Collisions between `Distinct` values for different Sets are possible with extremely high cardinality (billions of series per instrument), but are highly unlikely. (#7175) - `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/trace` synchronously de-duplicates the passed attributes instead of delegating it to the returned `TracerOption`. (#7266) - `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/meter` synchronously de-duplicates the passed attributes instead of delegating it to the returned `MeterOption`. (#7266) - `WithInstrumentationAttributes` in `go.opentelemetry.io/otel/log` synchronously de-duplicates the passed attributes instead of delegating it to the returned `LoggerOption`. (#7266) - Rename the `OTEL_GO_X_SELF_OBSERVABILITY` environment variable to `OTEL_GO_X_OBSERVABILITY` in `go.opentelemetry.io/otel/sdk/trace`, `go.opentelemetry.io/otel/sdk/log`, and `go.opentelemetry.io/otel/exporters/stdout/stdouttrace`. (#7302) - Improve performance of histogram `Record` in `go.opentelemetry.io/otel/sdk/metric` when min and max are disabled using `NoMinMax`. (#7306) - Improve error handling for dropped data during translation by using `prometheus.NewInvalidMetric` in `go.opentelemetry.io/otel/exporters/prometheus`.⚠️ **Breaking Change:** Previously, these cases were only logged and scrapes succeeded. Now, when translation would drop data (e.g., invalid label/value), the exporter emits a `NewInvalidMetric`, and Prometheus scrapes **fail with HTTP 500** by default. To preserve the prior behavior (scrapes succeed while errors are logged), configure your Prometheus HTTP handler with: `promhttp.HandlerOpts{ ErrorHandling: promhttp.ContinueOnError }`. (#7363) - Replace fnv hash with xxhash in `go.opentelemetry.io/otel/attribute` for better performance. (#7371) - The default `TranslationStrategy` in `go.opentelemetry.io/exporters/prometheus` is changed from `otlptranslator.NoUTF8EscapingWithSuffixes` to `otlptranslator.UnderscoreEscapingWithSuffixes`. (#7421) - Improve performance of concurrent measurements in `go.opentelemetry.io/otel/sdk/metric`. (#7427) - Include W3C TraceFlags (bits 0–7) in the OTLP `Span.Flags` field in `go.opentelemetry.io/exporters/otlp/otlptrace/otlptracehttp` and `go.opentelemetry.io/exporters/otlp/otlptrace/otlptracegrpc`. (#7438) - The `ErrorType` function in `go.opentelemetry.io/otel/semconv/v1.37.0` now handles custom error types. If an error implements an `ErrorType() string` method, the return value of that method will be used as the error type. (#7442) ### Fixed - Fix `WithInstrumentationAttributes` options in `go.opentelemetry.io/otel/trace`, `go.opentelemetry.io/otel/metric`, and `go.opentelemetry.io/otel/log` to properly merge attributes when passed multiple times instead of replacing them. Attributes with duplicate keys will use the last value passed. (#7300) - The equality of `attribute.Set` when using the `Equal` method is not affected by the user overriding the empty set pointed to by `attribute.EmptySet` in `go.opentelemetry.io/otel/attribute`. (#7357) - Return partial OTLP export errors to the caller in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`. (#7372) - Return partial OTLP export errors to the caller in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#7372) - Return partial OTLP export errors to the caller in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc`. (#7372) - Return partial OTLP export errors to the caller in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`. (#7372) - Return partial OTLP export errors to the caller in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`. (#7372) - Return partial OTLP export errors to the caller in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`. (#7372) - Fix `AddAttributes`, `SetAttributes`, `SetBody` on `Record` in `go.opentelemetry.io/otel/sdk/log` to not mutate input. (#7403) - Do not double record measurements of `RecordSet` methods in `go.opentelemetry.io/otel/semconv/v1.37.0`. (#7655) - Do not double record measurements of `RecordSet` methods in `go.opentelemetry.io/otel/semconv/v1.36.0`. (#7656) ### Removed - Drop support for [Go 1.23]. (#7274) - Remove the `FilterProcessor` interface in `go.opentelemetry.io/otel/sdk/log`. The `Enabled` method has been added to the `Processor` interface instead. All `Processor` implementations must now implement the `Enabled` method. Custom processors that do not filter records can implement `Enabled` to return `true`. (#7639)
4 participants
WithInstrumentationAttributes is creating a closure with a reference to a slice which is later passed to attribute.NewSet.
attribute.NewSet may mutate the slice, so this will lead to a data race when the option is applied concurrently. We can fix this by moving the call to attribute.NewSet outside the closure.
Fixes #7217
benchstat for New*Config