Make OTLP http/json stable

[carlosalberto]

We need to finally do some work to make OTLP http/json stable. From https://github.com/open-telemetry/opentelemetry-proto/issues/230#issuecomment-775307405 we need to:

• [x] #2796
• [x] Agree to no longer use the deprecated delimiter in proto, as this breaks JSON.
• [x] #2797
• [x] https://github.com/open-telemetry/opentelemetry-proto/issues/378
• [x] https://github.com/open-telemetry/opentelemetry-proto/issues/412
• [x] https://github.com/open-telemetry/opentelemetry-proto/issues/424
• [x] https://github.com/open-telemetry/opentelemetry-proto/issues/425
• [x] #2795

Probably supersedes #1252

[jmacd]

I would propose to see OTLP version information at the top-level of the structs passed to the collector. Appearing at the top-level of a struct makes it relatively simple for a data processor to interpret that value before parsing the rest of the structure.

For a JSON payload,

{ "otlp_version": "0.9", "resource_metrics": [versioned content] }

As an example of a recent situation where a non-breaking change broke something, the new-in-v0.10 metric staleness flag (when it was implemented in OTC v0.44) appeared to produce empty data points at our SaaS. Unless you're aware of the new flag, the data looks broken.

[tigrannajaryan]

As an example of a recent situation where a non-breaking change broke something, the new-in-v0.10 metric staleness flag (when it was implemented in OTC v0.44) appeared to produce empty data points at our SaaS. Unless you're aware of the new flag, the data looks broken.

@jmacd was any information present in the message that would allow you to know that the received data includes a staleness flag? If that is the case then it was a matter of using the information, right? If it was not present then I guess it means the change was done in backwards incompatible manner and it was expected that it would break recipients.

I am not strongly opposed to having the version number, but would like to better understand what problem we are solving by having it and in what scenarios exactly it is useful, how the recipients are supposed to be acting on the version number.

[tigrannajaryan]

I opened a separate issue to discuss the version number addition: https://github.com/open-telemetry/opentelemetry-proto/issues/378

[bogdandrutu]

@carlosalberto some questions:

Agree to no longer use the deprecated delimiter in proto, as this breaks JSON.

Is this the rename with a "deprecated_" prefix? Proto also has an option "@deprecated", which one are you referring to?

[tigrannajaryan]

Agree to no longer use the deprecated delimiter in proto, as this breaks JSON.

I am not sure what this is about. Does anyone know what the problem is?

[dyladan]

@carlosalberto some questions:

Agree to no longer use the deprecated delimiter in proto, as this breaks JSON.

Is this the rename with a "deprecated_" prefix? Proto also has an option "@deprecated", which one are you referring to?

By @deprecated do you mean the deprecated field option? The proto syntax for that is int32 old_field = 6 [deprecated = true];. In Java it translates to an @Deprecated annotation. According to the docs it has no effect in most languages so I'm not sure how it would break JSON. My assumption is that deprecated fields would be serialized into JSON and deserialized just like regular fields.

[tigrannajaryan]

According to the docs it has no effect in most languages so I'm not sure how it would break JSON. My assumption is that deprecated fields would be serialized into JSON and deserialized just like regular fields.

That's my understanding as well. It should have no effect on the wire or on the generated code. It is purely an annotation for humans (I may be wrong). The Protobuf spec that @dyladan refers to says:

deprecated (field option): If set to true, indicates that the field is deprecated and should not be used by new code. 
In most languages this has no actual effect. In Java, this becomes a @Deprecated annotation.

So I don't see how this can impact OTLP/JSON in some special way that we need to take into account.

[tigrannajaryan]

I am marking "Agree to no longer use the deprecated delimiter in proto, as this breaks JSON." as done. Please unmark if you think any additional work is needed.

[tigrannajaryan]

After merging https://github.com/open-telemetry/opentelemetry-specification/pull/2758 the enum names can no longer be used in OTLP/JSON so I am removing https://github.com/open-telemetry/opentelemetry-proto/issues/363 from this checklist, it is no longer relevant for OTLP/JSON (although still relevant for OTLP's generated code, so I am keeping it open).

[tigrannajaryan]

Verify that the current JSON protocol meets our needs.

I am going to remove this since it does not define any specific requirement. Please add back if you have thoughts on what we need here.

[tigrannajaryan]

The resolution of https://github.com/open-telemetry/opentelemetry-proto/issues/430 may impact JSON field name, so adding this to the checklist.

[Aneurysm9]

All tasks associated with this issue have been completed. Are there any remaining tasks that must be completed before OTLP/JSON can be stabilized?

[tigrannajaryan]

Are there any remaining tasks that must be completed before OTLP/JSON can be stabilized?

I think we are ready to declare it stable. Needs a PR that describes what exactly is guaranteed. It will be a subset of guarantees in https://github.com/open-telemetry/opentelemetry-proto/pull/432

[tigrannajaryan]

All work on making OTLP HTTP/JSON stable is now complete. The document is marked Stable as well. Closing this issue.