Do NOT guarantee string enum values backwards compatibility in JSON

[bogdandrutu]

See open-telemetry/opentelemetry-proto-go#81

If we declare that enums can be sent as strings, then we are blocking all enum values set, since the JSON decoding for enum strings that are not present causes error and messages will be rejected by the receivers, we will have issues rolling out any change to the enum values.

Because of this, I strongly suggest to not guarantee JSON string encoding for enums.

[bogdandrutu]

/cc @tigrannajaryan for where you keep track of issues for JSON stability.

[tigrannajaryan]

we will have issues rolling out any change to the enum values.

I am not sure I fully understand what the issue is. Do you mean that if in the future we want to add new values to existing enums we will not be able because it will cause old receivers to reject the message because they don't know how to interpret the new enum value? I think this is a desirable behavior. Adding a new enum value should be considered a backward incompatible change, even when using binary Protobuf. It is not just a JSON encoding issue and not just an issue with enum names. If the receiver is not aware of the new enum value the fact that it can decode the numeric value is not useful because the receiver has no idea what to do with that numeric value. (The only exception to this is receivers which part of "passthrough" services, which don't need to interpret the values - however I would not want to fixate on this very narrow use case).

So, to reiterate, in my opinion, addition of new values to existing enums must be considered a breaking change. Because of this there is no need to have any special rules in JSON encoding that enable such addition in a way that allows receivers to silently avoid failing on such new enum values. We should just prohibit changes like this once OTLP is declared 1.0.

[bogdandrutu]

Adding a new enum value should be considered a backward incompatible change, even when using binary Protobuf. It is not just a JSON encoding issue and not just an issue with enum names. If the receiver is not aware of the new enum value the fact that it can decode the numeric value is not useful because the receiver has no idea what to do with that numeric value. (The only exception to this is receivers which part of "passthrough" services, which don't need to interpret the values - however I would not want to fixate on this very narrow use case).

What???? Then we should not declare ever stability. This is a huge limitation that we add, for example we cannot add a new AggregationTemporality or a new SpanKind, I don't think this is acceptable.

[bogdandrutu]

Also, we are allowed to add new values for Enums declared in the API/SDK, we MUST allow that here as well.. That's why implementers can decide what to do on "unknown" value.

[tigrannajaryan]

I am open to be convinced otherwise. :-)

Do you suggest that we explicitly make it allowed to add new values to enums? How should the receivers work when they see an enum value they don't understand (let's talk about numeric values for now). For this to work we need to specify in OTLP that receivers that see unknown enum values must ignore such values and stop interpreting other related fields.

[bogdandrutu]

What do we do right now if we add a new SpanKind in an exporter? We do the same thing, there is no difference if I am local to the SDK or remote.

[tigrannajaryan]

Chatted with @bogdandrutu about this.

We agreed that it is important to be able to extend enums in the future by adding new values (e.g. new SpanKind is a good example). However, to be able to do so means old receivers need a good way to safely ignore such new values. While ignoring is easily doable when the message is received as binary Protobuf (you just keep the unknown numeric value of the field as is without trying to interpret it), it is not easily doable when the message received in JSON with enum value specified as the string equal to enum value's name. We need to be able to preserve such messages and pass them through unchanged in the Collector and there is really not reasonable way to do in the Collector codebase.

Because of this the proposal I think they only thing we can do is to disallow the use enum value strings in OTLP/JSON encoding. Only numeric values of the enum values may be used in JSON messages.

@open-telemetry/specs-approvers what do you think?

[tigrannajaryan]

Also, @open-telemetry/javascript-approvers what do you think (see my previous comment), this especially affects JS sources.

[jmacd]

there is really not reasonable way to do in the Collector codebase.

Could you expand on this? I'm not following the technical challenge. I wonder if OTLP enums can be represented with string type in the pdata packages, to allow unknown values to pass through.

[bogdandrutu]

I wonder if OTLP enums can be represented with string type in the pdata packages, to allow unknown values to pass through.

This does not work since you will break proto binary representation which accepts only numbers.

[jmacd]

We've already created special JSON-encoding rules for OTLP, so I wonder if we could specify that enums in JSON should be formatted as NAME(TAGNUM) to work around this?

[bogdandrutu]

@jmacd that is an option, or alternative support only number representation (already defined by protobuf as the tagnum). We loose a bit of human readability, but don't have to define a new format.

[tigrannajaryan]

I would prefer to prohibit usage of strings for enums. It is a slight loss of readability but saves a lot of trouble in the implementation.

[dyladan]

/cc @Aneurysm9 who has voiced opinions on this in the past

In JS we already switched to using numbers for enums a while ago and nobody has complained. I don't even know if anyone noticed. The human readability is slightly lost but we sidestep all the name stability and backwards compatibility problems. That said, I think it's a real bummer that we have to disallow using strings. IMO receiving an unknown string should be no different than receiving an unknown number and being able to change the value names for an enum seems to be extremely limited value to me.

I can see why the proto library you use in go made this decision, because the value is represented internally as an int. Representing an unknown int is easy because no type conversion is necessary. Representing an unknown string is impossible because there is no int it can be converted to that is guaranteed not to be used as an enum value in the future. I assume other strongly typed languages like Java may also have the same problem, but JS does not and I think it is likely Python, Ruby, etc also does not (in JS it is just undefined if an unknown string is used).

It would be possible to have some sort of middleware processing step in front of the unmarshaler which converted strings to numbers. There are only a handful of enums used in the proto and all of them have an UNSPECIFIED or UNKNOWN value.

Short version: I wouldn't block this but I'm not that happy about it either.

[bogdandrutu]

I can see why the proto library you use in go made this decision, because the value is represented internally as an int. Representing an unknown int is easy because no type conversion is necessary. Representing an unknown string is impossible because there is no int it can be converted to that is guaranteed not to be used as an enum value in the future. I assume other strongly typed languages like Java may also have the same problem, but JS does not and I think it is likely Python, Ruby, etc also does not (in JS it is just undefined if an unknown string is used).

Is not only go that has this, even Java has an enum called UNKNOWN which has an int in the representation. The idea behind is to be able to store/passthrough when encoding as "proto binary" which requires int representation. So the "int" representation is the common denominator between JSON and Proto binary.

[tigrannajaryan]

I created a PR to disallow enum value names in OTLP/JSON: open-telemetry/opentelemetry-specification#2758
Please review.