TC Feedback Request: View attribute filter definition in Go

[MrAlias]

Currently in Go we define the Stream parameter used to create a View using a slice of attribute keys for an allow-list:

https://github.com/open-telemetry/opentelemetry-go/blob/d78820e9050cd63daebdb4b82202f10d9c2b66e3/sdk/metric/instrument.go#L154

This is to strictly comply with the specification, which states a view needs to accept as a parameter:

attribute_keys: A list of attribute keys that MUST be kept, if provided by the user during the measurement, for the metric stream. All attributes with keys other than those in the list MUST be ignored.

This was recently changed away from a Stream definition where a user defined filter function was used to filter attributes:

https://github.com/open-telemetry/opentelemetry-go/blob/e0852d609c4a4205d550e2de45afdbf80d43557b/sdk/metric/instrument.go#L149

After making this change, we realized that we already have users where this new restrictive definition of attribute filtering will not help them. For example, we have seen users use a view to create a deny-list to filter out high-cardinality attributes from "troublesome" instrumentation. This is possible with a user-defined filter function, but will be difficult to support if we only accept attribute keys for an allow-only-list.

Because of this reduced functionality that will not solve our user's problems, we have staged changes to switch back to the prior definition of a Stream attribute filter while adding new filter creation functions for the user. One of these creation functions allows a user to provide a list of attribute keys and create an allow-only-list filter. I.e.

// This will only allow the attributes with "safe-key" as their key.
NewView(/* match criteria */, Stream{AttributeFilter: attribute.NewAllowKeysFilter("safe-key")})

We think this seems to be in the spirit of the specification, but the Go SIG would like a TC member review of the approach so we don't have to switch back later.

Will this be a compliant implementation?

Does the specification need to be modified to accommodate this?

cc @reyang @jsuereth @jmacd

Additional Context

@jack-berg pointed out that having a user-defined filter function in the stream definition is not unique to Go. The Java implementation uses one as well:

https://github.com/open-telemetry/opentelemetry-java/blob/bebf684436b0f54c0c81b19eba1dd9d12ff37532/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/ViewBuilder.java#L67-L77

However, one difference there is that they define the filter function to receive only the attribute key instead of the key-value pair.

[jmacd]

I think this is OK.

OTel-Go's attribute.Filter definition allows a programmer to modify the list of attributes by key and value, which is more flexible than an allow-list or deny-list by key name. This allows behavior that can't be configured easily, and that seems OK too.

I could imagine ways of abusing this feature, like use a non-deterministic filter function, that would lead to irregular behavior. I think generally filters should return a consistent set of keys across the process lifetime, but I would simply recommend against it, not require the SDK to perform extra validation.

@MrAlias would you prefer a more restrictive specification, that admits the allow-list and deny-list of attribute keys, such that could be configured through a yaml file easily, for example?

Also I wonder what @asafm thinks, in the context of #3566.

[MrAlias]

@MrAlias would you prefer a more restrictive specification, that admits the allow-list and deny-list of attribute keys, such that could be configured through a yaml file easily, for example?

I prefer the way OTel-Go had been doing it: with the filter function. I see it the same way you do, it allows users full functionality which is a benefit. The ability for a user to abuse the telemetry system will always be there, we just need to assume reasonably intelligent users will try to make the thing they need and not get in their way. It would not be great if a user needs complex filtering logic for their business purposes or to filter out PII and we are not able to allow that.

As for the configuration file, the filter approach will work with that as well. We can just pass the file configured allow-list (or deny-list) to the NewAllowKeysFilter (or NewDenyKeysFilter) function(s) when processing the configuration and produce the View the user configures.

The thing I need answered though is if this approach will comply with the specification given the field needs NewAllowKeysFilter to be used to exactly match the behavior in the specification. Does the specification need to be updated? Or, is this already compliant?

[jsuereth]

The intention of the specification was that the "hard list of keys" was the absolute minimum, but that filter functions would be allows as long as a hard list of keys also still works. This LGTM.

The open question of whether all key-value pairs is filtered or just the keys is a good question. For no good reason I'd prefer just keys, but I think what you have is fine and abides by the specification.

[asafm]

The only thing I have to say here is that for a new comer like me, to the specifications, I thought attribute_keys are not a minimum but the actual definition. Only when you approach the SDK (Java e.g.) you see that the definition is actually to select which attribute you wish to keep.
I don't know how it works, but if possible I would refine the specifications to mention it's either "the minimum" as was explained, or change the definition to say that the type is basically a filter to select the attributes to be included.

[MrAlias]

The only thing I have to say here is that for a new comer like me, to the specifications, I thought attribute_keys are not a minimum but the actual definition. Only when you approach the SDK (Java e.g.) you see that the definition is actually to select which attribute you wish to keep. I don't know how it works, but if possible I would refine the specifications to mention it's either "the minimum" as was explained, or change the definition to say that the type is basically a filter to select the attributes to be included.

#3680