-
Notifications
You must be signed in to change notification settings - Fork 1.2k
trace,metric,log: add WithInstrumentationAttributeSet option #7287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
trace,metric,log: add WithInstrumentationAttributeSet option #7287
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:
|
axw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :)
dashpole
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to support setting attributes with multiple options. WithInstrumentaitonAttributeSet should not clear any previously set attributes, and similar for `WithInstrumentationAttributes.
Co-authored-by: Tyler Yahn <[email protected]>
This would be a breaking behavioral change for |
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 <[email protected]>
|
#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
## 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