open-telemetry / opentelemetry-proto

OpenTelemetry protocol (OTLP) specification and Protobuf definitions
https://opentelemetry.io/docs/specs/otlp/
Apache License 2.0
606 stars 263 forks source link

Allow consumers to use map<string, AnyValue> instead of repeated KeyValue #515

Open punya opened 11 months ago

punya commented 11 months ago

OTel protos use repeated KeyValue or KeyValueList instead of proto3 map<string, AnyValue>, inconveniencing users:

My understanding is that we chose to use repeated because some of our tooling didn't understand the proto3 map syntax. Luckily map is just syntactic sugar for repeated and the documentation says:

Changing a field between a map<K, V> and the corresponding repeated message field is binary compatible (see Maps, below, for the message layout and other restrictions). However, the safety of the change is application-dependent: when deserializing and reserializing a message, clients using the repeated field definition will produce a semantically identical result; however, clients using the map field definition may reorder entries and drop entries with duplicate keys.

I realize that changing the protos to use maps at this point would break source-level backwards compatibility and be very disruptive to the community.

But could we provide a mechanically-generated variant of the protobufs that use maps instead, for people who want to use them?

This way,

tigrannajaryan commented 11 months ago

But could we provide a mechanically-generated variant of the protobufs that use maps instead, for people who want to use them?

I would rather not do it. I prefer to minimize the complexity here.

people would feel confident using them (rather than patching the protos manually)

What is the reason people who need this can't do the patching themselves?

jsuereth commented 11 months ago

What is the reason people who need this can't do the patching themselves?

A better question would be - Is this the default experience we want to give users of OTLP? We already patch for the collector in various scenarios where it's proto generation doesn't support proto3 concepts.

I would rather not do it. I prefer to minimize the complexity here.

What complexity are we optimising for? Today, we require users to understand they need to handle key conflicts themselves. Declaring it a map actually would let proto codegen do that for them.

I'm not suggesting we SHOULD make this change, but I do think there's larger concerns here. I'm more worried about generated code stability and overall user experience of generated code. From that view I see the trade offs as:

tigrannajaryan commented 11 months ago

We already patch for the collector in various scenarios where it's proto generation doesn't support proto3 concepts.

That is not the only reason we patch. We also need to add Gogoproto-specific annotations. This to me shows that people's needs for patching may be different. I think it is a slippery slope to try to publish all possible permutations of patched versions for everyone's needs. I don't see the data that tells this specific variety of the proto (with maps) is highly desirable and must be published by us and for example the patching needed by the Collector is not.

From that view I see the trade offs as...

Also maps are slower to serialize/deserialize (I benchmarked in Go).