-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Stabilize exemplars #5249
Comments
We need to export an |
We do not comply with this. |
We do not comply with this. |
Our current implementation complies with this. |
We do not comply with this. |
We do not comply with this. This is a clarification of #5249 (comment). |
Our implementation supports these filters. |
opentelemetry-go/sdk/metric/internal/exemplar/reservoir.go Lines 14 to 33 in 7ee6ff1
We comply with this. |
We comply with this. |
We comply with these. |
opentelemetry-go/sdk/metric/exemplar.go Lines 28 to 67 in 7ee6ff1
We comply with this. |
We comply with this:
|
We comply with this: https://github.com/open-telemetry/opentelemetry-go/blob/7ee6ff19b51eb4bffdd48639ac5698c9ee8932d6/sdk/metric/internal/exemplar/hist.go |
We do not comply with this. |
This is going to be a challenge. The Not sure how we can restructure the |
To address the generics, we can create a type ValueType uint8
const (
UnknownValueType ValueType = 0
Int64ValueType ValueType = 1
Float64ValueType ValueType = 2
)
type Value struct {
t ValueType
val uint64
}
func NewValue[N int64 | float64](val N) Value {
switch v := any(val).(type) {
case int64:
return newInt64Value(v)
case float64:
return newFloat64Value(v)
}
return Value{}
}
func newInt64Value(val int64) Value {
return Value{t: Int64ValueType, val: uint64(val)}
}
func newFloat64Value(val float64) Value {
return Value{t: Float64ValueType, val: math.Float64bits(val)}
}
func (v Value) Type() ValueType { return v.t }
func (v Value) Int64() int64 {
if v.t == Int64ValueType {
return v.int64()
}
return 0
}
func (v Value) int64() int64 { return int64(v.val) }
func (v Value) Float64() float64 {
if v.t == Float64ValueType {
return math.Float64frombits(v.val)
}
return 0
}
func (v Value) float64() float64 { return math.Float64frombits(v.val) }
func (v Value) Any() any {
switch v.t {
case Int64ValueType:
return v.int64()
case Float64ValueType:
return v.float64()
}
return nil
} From there we can define an exemplar: // Exemplar is a measurement sampled from a timeseries providing a typical
// example.
type Exemplar struct {
// FilteredAttributes are the attributes recorded with the measurement but
// filtered out of the timeseries' aggregated data.
FilteredAttributes []attribute.KeyValue
// Time is the time when the measurement was recorded.
Time time.Time
// Value is the measured value.
Value Value
// SpanID is the ID of the span that was active during the measurement. If
// no span was active or the span was not sampled this will be empty.
SpanID []byte `json:",omitempty"`
// TraceID is the ID of the trace the active span belonged to during the
// measurement. If no span was active or the span was not sampled this will
// be empty.
TraceID []byte `json:",omitempty"`
} and a // Reservoir holds the sampled exemplar of measurements made.
type Reservoir interface {
// Offer accepts the parameters associated with a measurement. The
// parameters will be stored as an exemplar if the Reservoir decides to
// sample the measurement.
//
// The passed ctx needs to contain any baggage or span that were active
// when the measurement was made. This information may be used by the
// Reservoir in making a sampling decision.
//
// The time t is the time when the measurement was made. The val and attr
// parameters are the value and dropped (filtered) attributes of the
// measurement respectively.
Offer(ctx context.Context, t time.Time, val Value, attr []attribute.KeyValue)
// Collect returns all the held exemplars.
//
// The Reservoir state is preserved after this call.
Collect(dest *[]Exemplar)
} |
I'm working to refactor |
Moving to the v1.28.0 milestone as #5285 needs to be released and used before that code is exported to resolve this. |
Blocked by #5542 |
The plan is to try and get a PR merged with the new public interface right after the next release. That way we have a longer time to evaluate the change before it becomes stable (:grimacing:). |
Part of #5249. Addresses #5249 (comment) This removes handling of the `OTEL_GO_X_EXEMPLAR` environment variable. Instead of changing the default for the existing environment variable to enable it by default, i'm just removing it entirely. Users can still disable the feature by setting the filter to always_off. Since we will continue to support that configuration, it seems better to direct users there, rather than give them a temporary equivalent.
Part of #5249 This makes all existing types designed to implement the public Exemplar API public by moving most of `internal/exemplar` to `exemplar`. The only types that are not being made public are `exemplar.Drop`, and `exemplar.FilteredReservoir`. Those types are moved to `internal/aggregate`, and are renamed to `DropReservoir` and `FilteredExemplarReservoir`. The following types are made public: * `exemplar.Exemplar` * `exemplar.Filter` * `exemplar.SampledFilter` * `exemplar.AlwaysOnFilter` * `exemplar.HistogramReservoir` * `exemplar.FixedSizeReservoir` * `exemplar.Reservoir` * `exemplar.Value` * `exemplar.ValueType`
Looking into stream configuration, there are a few different ways we could approach it. Our Option 1: type ExemplarReservoirProvider func() exemplar.Reservoir Option 2: type ExemplarReservoirProvider func(agg Aggregation) exemplar.Reservoir The text of the specification is:
If we choose Option 1, we can't implement the "default reservoir provider" as an ExemplarReservoirProvider, which feels strange. We would have a ExemplarReservoirProvider-Provider that provides a different provider based on the aggregation. With option 1 users can still effectively do the same thing as with option 2. If they plan to override the default aggregation, they can simply provide the appropriate reservoir for that aggregation. If they do not plan to override the default aggregation, they can invoke DefaultAggregationSelector on the instrument kind to get it. But that feels like a lot of work when we already have it. The fact that we will need to use the Aggregation when choosing our exemplar Reservoir makes me think we should go with Option 2. It is also nice to be able to provide the default reservoir provider to users so they can use it as a fallback. |
I've prototyped option 2 in #5861 |
More importantly, I've confirmed in #5861 that we can implement stream configuration without requiring any changes to the public types in |
Starting a new audit of everything except stream+ filter configuration:
opentelemetry-go/sdk/metric/exemplar/exemplar.go Lines 12 to 29 in d7e7da6
We comply with this. |
opentelemetry-go/sdk/metric/exemplar.go Lines 35 to 36 in d7e7da6
We comply with this. |
Our exemplar.Filter is a function, which the Note that the AlwaysOffFilter is being added in #5850, and would also be an option. The only question I have is whether |
opentelemetry-go/sdk/metric/exemplar/reservoir.go Lines 13 to 32 in d7e7da6
We provide all of those, EXCEPT we provide the filtered set of attributes, rather than the complete set. This is acceptable given that "The offer method MAY accept a filtered subset of Attributes" below.
This can be done by inspecting the Go
The interface does not prevent users from handling measurements in the way they want to.
We do accept a filtered subset of attributes, but DO NOT comply with this:
We do not currently comply with this, but this only applies to custom reservoir implementations. See #5861 for how this could be implemented without changes to any of the existing functions.
Our collect function does not reset internal storage for delta temporal aggregation that I can tell. @MrAlias this differs from your analysis here: #5249 (comment). We do not comply with this
We comply with this, since offer provides filtered attributes.
We comply with this. Within a single aggregation, a new reservoir is created (note the
We do our best :), but this should not depend on the interface definitions. |
We've chosen to use simpler names than the specification recommends, but for good reason:
|
opentelemetry-go/sdk/metric/exemplar.go Lines 39 to 78 in 4ac842c
We are compliant. |
Our current exemplar.Reservoir interface would allow for user-provided implementations:
|
Based on: #4873 Which was based on #4871 (comment) We only ever will return exemplars from the most recent time interval. The reservoir is destroyed each cycle. Given this is a recommendation and we use the more "optimal implementation" of deleting the reservoir for delta temporality, it seems like a valid case to not comply. |
That's a good catch. This was something imposed on us by other implementations that only accept the full attribute sets for I'm still not sold it is needed given the concept of an attribute filter exists, and none of these attributes are needed by our built-in reservoirs. I think your work-around in #5861 is a good solution though. And will ensure we are compliant 👍 |
Part of #5249 ### Spec https://opentelemetry.io/docs/specs/otel/metrics/sdk/#exemplarfilter > The ExemplarFilter configuration MUST allow users to select between one of the built-in ExemplarFilters. While ExemplarFilter determines which measurements are eligible for becoming an Exemplar, the ExemplarReservoir makes the final decision if a measurement becomes an exemplar and is stored. > The ExemplarFilter SHOULD be a configuration parameter of a MeterProvider for an SDK. The default value SHOULD be TraceBased. The filter configuration SHOULD follow the [environment variable specification](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#exemplar). > An OpenTelemetry SDK MUST support the following filters: > * [AlwaysOn](https://opentelemetry.io/docs/specs/otel/metrics/sdk/#alwayson) > * [AlwaysOff](https://opentelemetry.io/docs/specs/otel/metrics/sdk/#alwaysoff) > * [TraceBased](https://opentelemetry.io/docs/specs/otel/metrics/sdk/#tracebased) ### Changes * adds exemplar.AlwaysOffFilter, which is one of the required filters from the SDK: https://opentelemetry.io/docs/specs/otel/metrics/sdk/#alwaysoff * adds `metric.WithExemplarFilter` as an option for the metrics SDK. * moves handling of `OTEL_METRICS_EXEMPLAR_FILTER` to the same location as config handling to make code easier to navigate. dropReservoir can actually be removed, but I plan to do that in a follow-up refactor, since it will be a large diff. --------- Co-authored-by: Damien Mathieu <[email protected]> Co-authored-by: Tyler Yahn <[email protected]> Co-authored-by: Chester Cheung <[email protected]>
Topic: #5249 This reverts commit 8041156 (PR: #5899) due to the performance degradation found by Benchmarks CI https://github.com/open-telemetry/opentelemetry-go/actions/runs/11447364022/job/31848519243 Here is the benchmark test on my machine: ``` goos: darwin goarch: arm64 pkg: go.opentelemetry.io/otel/sdk/metric │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ Instrument/instrumentImpl/aggregate-10 3.378µ ± 3% 49.366µ ± 1% +1361.40% (p=0.000 n=10) Instrument/observable/observe-10 2.288µ ± 2% 37.791µ ± 1% +1551.73% (p=0.000 n=10) geomean 2.780µ 43.19µ +1453.65% │ old.txt │ new.txt │ │ B/op │ B/op vs base │ Instrument/instrumentImpl/aggregate-10 1.245Ki ± 1% 22.363Ki ± 0% +1696.08% (p=0.000 n=10) Instrument/observable/observe-10 823.0 ± 1% 17432.5 ± 0% +2018.17% (p=0.000 n=10) geomean 1.000Ki 19.51Ki +1850.48% │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ Instrument/instrumentImpl/aggregate-10 1.000 ± 0% 21.000 ± 0% +2000.00% (p=0.000 n=10) Instrument/observable/observe-10 1.000 ± 0% 16.000 ± 0% +1500.00% (p=0.000 n=10) ```
Exemplars are now stable in the specification.
Compliance TODOs
ExemplarReservoir
type and accept it as stream configurationExemplarFilter
to be a configured for aMeterProvider
with a default value ofTraceBased
(continue to support environment variable configuration of theExemplarFilter
)Stabilization procedure and plan
TODO:
The text was updated successfully, but these errors were encountered: