Deprecate old explicit bounds, add message for explicit bounds #255
Conversation
This is required in order for the protocol to develope in the future and support adding different other types of bounds. With this change it is backwards compatible to add multiple bound types and embed them into an oneof (wire compatible). On any service (receiver) if the deprecated field is received the only operation required is to reassign the deprecated field to the new embeded repeated field in the message. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
| @@ -496,24 +500,11 @@ message DoubleHistogramDataPoint { | |||
| // Otherwise all option fields and "buckets" field must be omitted in which case the | |||
| // distribution of values in the histogram is unknown and only the total count and sum are known. | |||
|
|
|||
| repeated double deprecated_explicit_bounds = 7 [deprecated=true]; | |||
There was a problem hiding this comment.
For my own understanding, we're not supporting JSON compatibility yet, so this kind of deprecation (with an immediate rename) is ok?
There was a problem hiding this comment.
This will also break Collector's build because the field name is used in the codebase, but that's acceptable for now.
What's probably more important is that once we update Collector translation code to use the new explicit_bounds field we are going to break semantic compatibility with existing senders / receivers. (Alternatively, we can look at the deprecated field as well, but not sure if we want to do that).
There was a problem hiding this comment.
I am deprecating the field and plan to accept it in the receiver and translate it into the new format
There was a problem hiding this comment.
I'm not sure this answered my question. Can we rename here? Does this break a JSON implementation?
I understand from a PROTO standpoint, this isn't breaking, but this would break text-based version (IIUC). Do we not have text-based usage yet so only proto matters?
There was a problem hiding this comment.
It does break JSON. This is acceptable, since JSON is currently declared Alpha: https://github.com/open-telemetry/opentelemetry-proto#maturity-level and we allow breaking changes while in Alpha.
|
I intend to send an alternate to this PR, in which I propose the approach described here: #259 (comment) Specifically, this would:
|
|
The group has discussed support for #272. |
This is required in order for the protocol to develope in the future and support adding
different other types of bounds. With this change it is backwards compatible to add multiple
bound types and embed them into an oneof (wire compatible).
On any service (receiver) if the deprecated field is received the only operation required
is to reassign the deprecated field to the new embeded repeated field in the message.
Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com