search

open-telemetry / opentelemetry-configuration

JSON Schema definitions for OpenTelemetry file configuration
Apache License 2.0
41 stars 17 forks source link

Should attribute_keys in stream configuration support include/exclude syntax? #98

Closed codeboten closed 3 months ago

codeboten commented 5 months ago

The specification states:

attribute_keys: This is, at a minimum, an allow-list of attribute keys for measurements captured in the metric stream. The allow-list contains attribute keys that identify the attributes that MUST be kept, and all other attributes MUST be ignored.

But looking at the go implementation, an attribute filter can be specified either to include or exclude based on the attribute keys:

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

I checked the python implementations, and it looks like that implementation only supports including attributes based on the keys.

tsloughter commented 5 months ago

Erlang/Elixir only supports including attributes based on key as well.

jack-berg commented 5 months ago

As a general rule, I don't think the file configuration schema should invent things that aren't in the spec, even if many languages provide something beyond what is specified. If users (or language maintainers) want something more expressive, the correct route seems to be to get the language changed in the spec, then reflect those changes in file config schema. If we didn't do this, then we would be potentially overpromising what is actually configurable. For example, suppose a language only supports include syntax because that's what the spec recommends, and file config supported include AND exclude syntax. A user would be try to use that configuration and it wouldn't work. And it would be hard to convince the language maintainer to add support for the exclude syntax because the spec doesn't mention it.

As I say this, there are examples where the spec is vague and we might consider filling in the details. For example, OTLP retry config... The spec never tackled this and doesn't seem like it will in the short term future. Perhaps file config should wait for the spec to describe those properties, or perhaps the spec never described those properties because it was constrained to flat env vars.

jack-berg commented 3 months ago

Discussed and decided that this is blocked on additional spec language for a deny list.

For reference, here's what the spec currently states on the topic:

https://github.com/open-telemetry/opentelemetry-specification/blame/0b3328bb17fa6f86af521f8d22dbbcddea2ac914/specification/metrics/sdk.md#L362-L368

jack-berg commented 3 months ago

Can we redefine the property schema we have today to allow for a more seamless addition of an exclude list in the future? Should do so before stability.