opentelemetry-proto
opentelemetry-proto copied to clipboard
open-telemetry
Rename AnyValue to Value, this should not be on the wire breaking change
Message names are not used in JSON or Protobuf encoding, so this is a no-op for both json and proto wire encoding.
Signed-off-by: Bogdan Drutu [email protected]
The "breaking change" steps seems super cool, but I fail to understand how that fails with this change. See https://github.com/open-telemetry/opentelemetry-proto/actions/runs/3070753656/jobs/4960806704
I understand now:
Choose a category based on these criteria:
If you distribute your generated source code outside of a monorepo in any capacity, or want to make
sure that consumers of the generated source code don't experience broken builds, use FILE or
PACKAGE. Choose FILE if you use (or might use) any languages that generate header files (such as
C++ or Python), or PACKAGE if you only use languages that generate code on a per-package basis
(such as Golang). If in doubt, choose FILE.
If you only use Protobuf within a monorepo and always re-generate, and are OK with refactoring code
that consumes the generated source code, use WIRE or WIRE_JSON. Use WIRE if you are sure that you
will never use the JSON representation of your Protobuf messages. Use WIRE_JSON if there is any JSON
usage. Generally, we'd recommend using WIRE_JSON if you go this route (this basically just has the effect
of not allowing re-use of field and enum value names).
We are using FILE as the default category, not sure about the expectations here. Back to the same question.
what is the motivation for this change? or is it just an experiment?
Initially I thought that the name may improve usability, so wanted to create a PR to hear opinions, but now I am intrigued about the rules.
We are using
FILEas the default category, not sure about the expectations here. Back to the same question.
We are probably using a wrong setting. I think we should use WIRE cause that's what we guarantee today.
As for the specific change I am not sure that the new name is better. If we ever add any other value type in the future it can become confusing. We also reference to any value in the spec (in e.g. log data model) so I think having AnyValue here also is better aligned with that.
Given that this change is also going to cause pains for users of the proto's generated code I would prefer to keep the current name.
@tigrannajaryan I was not sold on the name, I was hacking around to see how the new name looks and found that I break the requirements, and that triggered me to create the PR.
We are using FILE as the default category, not sure about the expectations here. Back to the same question.
It is set up to use WIRE: https://github.com/open-telemetry/opentelemetry-proto/blob/724e427879e3d2bae2edc0218fff06e37b9eb46e/buf.yaml#L6
I think the check just doesn't recognize the rename and so thinks the field type changed, see https://github.com/bufbuild/buf/issues/609. However, I suppose changing the name would be a wire breaking change if the message appears in an Any (which I don't think we use). Sorry for the noise.
@bogdandrutu anything further to discuss on this? I think the discovery is that tool is not properly detecting that it is just a renaming, so let's be aware of it for any future change.
However, it also seems that we are not going forward with the proposed renaming, so close the PR?