trace,metric,log: add WithInstrumentationAttributeSet option#7287
trace,metric,log: add WithInstrumentationAttributeSet option#7287pellared merged 14 commits intoopen-telemetry:mainfrom
Conversation
|
@axw PTAL |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7287 +/- ##
=====================================
Coverage 82.9% 82.9%
=====================================
Files 266 266
Lines 24948 24984 +36
=====================================
+ Hits 20698 20734 +36
Misses 3875 3875
Partials 375 375
🚀 New features to boost your workflow:
|
dashpole
left a comment
There was a problem hiding this comment.
Should we deprecate WithInstrumentationAttributes and point users toward WithInstrumentationAttributeSet? As a user it isn't clear which I should use, and this seems to be a safer+better option.
I was thinking about it as well! Especially that I do not think there are a lot of usages of I have double-check if there is a performance difference between: attrs := attribute.NewSet(attribute.String("service", "test"))
trace.NewTracerConfig(
metric.WithInstrumentationAttributeSet(attrs),
)and trace.NewTracerConfig(
trace.WithInstrumentationAttributes(attribute.String("service", "test")),
)Still, I would rather do it a separate PR as I think this is a little subjective whether it should be deprecated or not. It would be also easier to "revert commit" if we change our mind that deprecating Also please see (including conversations): #7266 EDIT: |
|
I'm OK with deferring discussions about deprecating the non-set option. |
MrAlias
left a comment
There was a problem hiding this comment.
We want to support setting attributes with multiple options. WithInstrumentaitonAttributeSet should not clear any previously set attributes, and similar for `WithInstrumentationAttributes.
I'm find scoping the fix to However, I'm hesitant to add another option that also contains this bug. Especially since it is adding a conflicting way to set the same value as an existing option. |
Let's fix |
…ibutes (#7300) ## What 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. ## Why Per #7287 (review) and #7287 (comment) Not that this does not address #7217. This is left to a seperate PR to not scope creep this one. CC @axw --------- Co-authored-by: Damien Mathieu <42@dmathieu.com>
|
#7300 is merged and this PR is updated (comments addressed). EDIT: I also added benchmarks to showcase the performance benefits. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds WithInstrumentationAttributeSet option functions to the log, metric, and trace packages to provide a concurrent-safe alternative to the existing WithInstrumentationAttributes functions by accepting pre-constructed attribute.Set instead of variadic attribute.KeyValue parameters.
- Adds new
WithInstrumentationAttributeSetfunctions that acceptattribute.Setfor better concurrency safety and performance - Updates documentation for existing
WithInstrumentationAttributesfunctions to recommend the new alternative - Implements comprehensive test coverage including merge behavior and benchmarks
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| trace/config.go | Adds WithInstrumentationAttributeSet function and updates documentation |
| trace/config_test.go | Adds tests and benchmarks for the new function |
| metric/config.go | Adds WithInstrumentationAttributeSet function and updates documentation |
| metric/config_test.go | Adds tests and benchmarks for the new function |
| log/logger.go | Adds WithInstrumentationAttributeSet function and updates documentation |
| log/logger_test.go | Adds tests and benchmarks for the new function |
| CHANGELOG.md | Documents the new feature addition |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
## 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)
Per #7266 (comment)
Related to #7217
What
This PR adds
WithInstrumentationAttributeSetoption functions to thelog,metric, andtracepackages as suggested in #7266 (comment). These new functions provide a more concurrent-safe alternative to the existingWithInstrumentationAttributesfunctions by accepting a pre-constructedattribute.Setinstead of variadicattribute.KeyValueparameters.Why
As discussed in #7266, the existing
WithInstrumentationAttributesfunctions can lead to data races when used concurrently becauseattribute.NewSet()may mutate the passed slice in-place. While the issue was partially addressed by moving theattribute.NewSet()call outside the closure, the best long-term solution is to provide an alternative that accepts an immutableattribute.Set.Benefits:
attribute.Setis immutable, these functions are inherently safe for concurrent useattribute.NewSet()when the same attributes are used multiple timesmetric.WithAttributeSet()Deprecating
WithInstrumentationAttributesis out of scope. See #7287 (comment).Benchmarks