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

Consistently declare Protobuf enums #115

Conversation

tigrannajaryan
Copy link
Member

Enums were not consistently declared. Some were at the global scope,
some others wire nested in messages. This moves all enums to the global
scope and uses most of the official style guide rules here:
https://developers.google.com/protocol-buffers/docs/style#enums

Unfortunately this results in long and stuttering names of enum values in
the code but this change is necessary to future-proof the proto and ensure
no enum names conflict when we add new enums and values (all enum value
names are in one namespace).

I don't like the end result but not doing it appears to be worse.
We have seen this first-hand in OTLP proto where enum names are
inconsistent and are now impossible to fix due to backward compatibility
concerns.

Note: I ignored the rule to CAPITALIZE names and use underscores
because if I do that the names in the code become unreadable walls
of text.

If anyone has any ideas how to fix the enum names issue in a nicer
way I am happy to discard this PR.

One alternate is to remove the prefixes from enum value names and
just require that all enum value names introduced in the future are
also unique, but I think it can result in weird names in the future.

@tigrannajaryan tigrannajaryan requested a review from a team July 29, 2022 14:25
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #115 (57922c7) into main (89eeb30) will increase coverage by 2.11%.
The diff coverage is 82.60%.

❗ Current head 57922c7 differs from pull request most recent head 3e0ac5c. Consider uploading reports for the commit 3e0ac5c to get more accurate results

@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
+ Coverage   74.61%   76.72%   +2.11%     
==========================================
  Files          23       23              
  Lines        1765     1706      -59     
==========================================
- Hits         1317     1309       -8     
+ Misses        334      298      -36     
+ Partials      114       99      -15     
Impacted Files Coverage Δ
client/internal/packagessyncer.go 59.25% <60.00%> (ø)
client/internal/clientcommon.go 81.00% <77.77%> (+1.19%) ⬆️
client/internal/inmempackagestore.go 76.47% <100.00%> (ø)
client/internal/receivedprocessor.go 84.28% <100.00%> (+15.00%) ⬆️
client/httpclient.go 94.73% <0.00%> (ø)
client/internal/httpsender.go 72.26% <0.00%> (+1.29%) ⬆️
server/serverimpl.go 60.65% <0.00%> (+2.28%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/consistent-proto branch from a06d155 to 57922c7 Compare August 3, 2022 16:48
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/consistent-proto branch from 57922c7 to 3b3e9fc Compare August 23, 2022 17:03
Enums were not consistently declared. Some were at the global scope,
some others wire nested in messages. This moves all enums to the global
scope and uses most of the official style guide rules here:
https://developers.google.com/protocol-buffers/docs/style#enums

Unfortunately this results in long and stuttering names of enum values in
the code but this change is necessary to future-proof the proto and ensure
no enum names conflict when we add new enums and values (all enum value
names are in one namespace).

I don't like the end result but not doing it appears to be worse.
We have seen this first-hand in OTLP proto where enum names are
inconsistent and are now impossible to fix due to backward compatibility
concerns.

Note: I ignored the rule to CAPITALIZE names and use underscores
because if I do that the names in the code become unreadable walls
of text.
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/consistent-proto branch from 3b3e9fc to 3e0ac5c Compare August 23, 2022 17:15
@tigrannajaryan
Copy link
Member Author

Rebased and fixed merge conflicts. @andykellr if you have time please take another look (just want to make sure I didn't miss anything), otherwise I will merge soon.

@tigrannajaryan tigrannajaryan merged commit dc8c620 into open-telemetry:main Sep 20, 2022
@tigrannajaryan tigrannajaryan deleted the feature/tigran/consistent-proto branch September 20, 2022 16:47
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.

2 participants