Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
punya opened this issue Nov 30, 2023 · 3 comments
Open

Comments

@punya
Copy link
Member

punya commented Nov 30, 2023

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,

  • The map-variants would be guaranteed to stay up to date, and people would feel confident using them (rather than patching the protos manually)
  • Binary-compatibility would mean that it's safe to use the two formats together
@tigrannajaryan
Copy link
Member

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
Copy link
Contributor

jsuereth commented Dec 4, 2023

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:

  • map<> representation seems easier to consume + less confusing/complex.
  • map<> representation seems more likely to cause compat issues across codgen versions.

@tigrannajaryan
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants