search

open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.28k stars 1.08k forks source link

Use view AttributeFilter #3390

Closed MrAlias closed 2 years ago

MrAlias commented 2 years ago

Currently the view.View type provides an AttributeFilter method and the related WithFilterAttributes option.

https://github.com/open-telemetry/opentelemetry-go/blob/ccbc38e66ede23ef04d86f690df5c7c525ecd4f5/sdk/metric/view/view.go#L90-L100

https://github.com/open-telemetry/opentelemetry-go/blob/ccbc38e66ede23ef04d86f690df5c7c525ecd4f5/sdk/metric/view/view.go#L196-L214

These are unused in the SDK.

MadVikingGod commented 2 years ago

I don't think we can just remove this. This is a feature described in the view specification.

When working on this I found that we have incorrect logic of the async sum aggregator. The scenario I have is if you have an async counter and a filter you can have two Observers that should be summed.

eg:

v, err := view.New(
    view.MatchInstrumentName("*"),
    view.WithFilterAttributes(attribute.Key("foo")),
)
...

mtr.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) {
    ctr.Observe(ctx, 10, attribute.Int("version", 1))
    ctr.Observe(ctx, 20, attribute.Int("version", 2))
})

I would expect this would return a sum of 30 on collect.

MrAlias commented 2 years ago

I don't think we can just remove this. This is a feature described in the view specification.

When working on this I found that we have incorrect logic of the async sum aggregator. The scenario I have is if you have an async counter and a filter you can have two Observers that should be summed.

eg:

v, err := view.New(
  view.MatchInstrumentName("*"),
  view.WithFilterAttributes(attribute.Key("foo")),
)
...

mtr.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) {
  ctr.Observe(ctx, 10, attribute.Int("version", 1))
  ctr.Observe(ctx, 20, attribute.Int("version", 2))
})

I would expect this would return a sum of 30 on collect.

Can you open an issue to track this bug?

MrAlias commented 2 years ago

Given the mentioned bug, and https://github.com/open-telemetry/opentelemetry-go/pull/3396#discussion_r1007236678

I think this bug is more subtle that just saying that it is in the sum aggregator (though, to be precise, it relates to the precomputedSum).

When a user passes a value to Observe it is interpreted as being the pre-compute cumulative sum value. If Observe is called twice in a callback and observed to be different values for the same attribute set, shouldn't the last value observed be used? It is the value the user is reporting the sum to be at. It seems to make sense in this situation that the last value is used.

However, the issue is the user didn't report the cumulative sum for the empty attribute set here, they report one for version 1 and one for 2. The question becomes, how should precomputed sums be combined when we know they are reported for different attribute sets? This means that the solution to this issue is going to revolve around the filter aggregation because that is the only place that know it is combining two distinct attribute sets (unless we redefine where attribute recording is done: https://github.com/open-telemetry/opentelemetry-go/issues/3030). The issue to resolve is it doesn't know it is combining them for the precomputedSum aggregator though.

We should probably look into how other SIGs are resolving this and if there is any specification on this behavior.

MrAlias commented 2 years ago

Specification issue related to behavior question: https://github.com/open-telemetry/opentelemetry-specification/issues/2905