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

Rename AnyValue to Value, this should not be on the wire breaking change #429

Closed
wants to merge 1 commit into from

Conversation

bogdandrutu
Copy link
Member

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 bogdandrutu@gmail.com

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu
Copy link
Member Author

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

@bogdandrutu
Copy link
Member Author

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.

@yurishkuro
Copy link
Member

what is the motivation for this change? or is it just an experiment?

@bogdandrutu
Copy link
Member Author

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.

@tigrannajaryan
Copy link
Member

We are using FILE as 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.

@bogdandrutu
Copy link
Member Author

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

@aabmass
Copy link
Member

aabmass commented Sep 21, 2022

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:

I think the check just doesn't recognize the rename and so thinks the field type changed, see bufbuild/buf#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.

@tigrannajaryan
Copy link
Member

@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?

@bogdandrutu bogdandrutu deleted the anyvalue branch January 24, 2023 19:45
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

Successfully merging this pull request may close these issues.

None yet

5 participants