Use view AttributeFilter

[MrAlias]

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

opentelemetry-go/sdk/metric/view/view.go

Lines 90 to 100 in ccbc38e

	// AttributeFilter returns a function that returns only attributes specified by
	// WithFilterAttributes. If no filter was provided nil is returned.
	func (v View) AttributeFilter() func(attribute.Set) attribute.Set {
	if v.filter == nil {
	return nil
	}
	return func(input attribute.Set) attribute.Set {
	out, _ := input.Filter(v.filter)
	return out
	}
	}

opentelemetry-go/sdk/metric/view/view.go

Lines 196 to 214 in ccbc38e

	// WithFilterAttributes will select attributes that have a matching key. If not used
	// or empty no filter will be applied.
	func WithFilterAttributes(keys ...attribute.Key) Option {
	return optionFunc(func(v View) View {
	if len(keys) == 0 {
	return v
	}
	filterKeys := map[attribute.Key]struct{}{}
	for _, key := range keys {
	filterKeys[key] = struct{}{}
	}
	v.filter = attribute.Filter(func(kv attribute.KeyValue) bool {
	_, ok := filterKeys[kv.Key]
	return ok
	})
	return v
	})
	}

These are unused in the SDK.

• Remove this method and option, or use this method in the SDK to filter attributes.

[MadVikingGod]

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]

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]

Given the mentioned bug, and #3396 (comment)

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: #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]

Specification issue related to behavior question: open-telemetry/opentelemetry-specification#2905