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

Tidy up (and document) .proto files #814

Open
NimJay opened this issue May 6, 2022 · 7 comments
Open

Tidy up (and document) .proto files #814

NimJay opened this issue May 6, 2022 · 7 comments
Labels
area/grpc priority: p3 Desirable enhancement or fix. May not be included in next release. type: cleanup An internal cleanup or hygiene concern.

Comments

@NimJay
Copy link
Collaborator

NimJay commented May 6, 2022

Describe request or inquiry

I have created this issue to request tidy up of (and/or documentation for) these two issues:

  1. .proto File Duplication - We might be able to tidy this up.
  2. Inconsistent use of .protos - Most — if not, all — of this inconsistency is inevitable (because gRPC code generation works differently for the various programming languages used by the Online Boutique microservices).

1. .proto File Duplication

2. Inconsistent Use of .protos

The way in which a microservice uses the .proto files and generates its gRPC code depends on the programming language of that microservice.
Ideally, gRPC (and .proto file) usage would be better documented for each microservice.
Here's some info about the overall issue, for now:

Microservice Language How are .protos used? Generated gRPC Code .proto file used
adservice Java /src/adservice/src/release/v0.3.6/proto/demo.proto
cartservice C# /src/protos/Cart.proto
checkoutservice Go See /src/checkoutservice/genproto.sh /src/checkoutservice/genproto/demo.pb.go /pb/demo.proto
currencyservice JavaScript gRPC code is generated during runtime, so we just need to feed the .proto file(s) into the gRPC library when we run the Node.js server. /src/currencyservice/proto/demo.proto
emailservice Python See /src/emailservice/genproto.sh /src/emailservice/demo_pb2.py & /src/emailservice/demo_pb2_grpc.py /pb/demo.proto
frontend Go See /src/frontend/genproto.sh /src/frontend/genproto/demo.pb.go /pb/demo.proto
loadgenerator Python The loadgenerator just makes HTTP requests to the frontend service. No gRPC is used.
recommendationservice Python See /src/recommendationservice/genproto.sh /src/recommendationservice/demo_pb2.py & /src/recommendationservice/demo_pb2_grpc.py /pb/demo.proto
shippingservice Go See /src/shippingservice/genproto.sh /src/shippingservice/genproto/demo.pb.go /pb/demo.proto
paymentservice JavaScript gRPC code is generated during runtime, so we just need to feed the .proto file(s) into the gRPC library when we run the Node.js server. /src/paymentservice/proto/demo.proto
productcatalogservice Go See /src/productcatalogservice/genproto.sh /src/productcatalogservice/genproto/demo.pb.go /pb/demo.proto

What purpose/environment will this feature serve?

  • Tidying up the .protos will help reduce the learning curve of this repo, and make it easier for new developers to contribute and use Online Boutique.

What inspired this issue?

  • A team at Google is using Online Boutique to showcase the Retail API. The team faced some friction while trying to refactor the .protos for the Python microservices and the frontend service.
@NimJay NimJay added type: cleanup An internal cleanup or hygiene concern. priority: p3 Desirable enhancement or fix. May not be included in next release. labels May 6, 2022
@mathieu-benoit
Copy link
Contributor

Could be great to consider this one in the meantime maybe? #1

@mic-max
Copy link

mic-max commented Jun 7, 2022

In OpenTelemetry we started a fork and have some work done to remove these duplicate .proto files as part of the following PRs:

Each service's Dockerfile copies from /pb/demo.proto and runs protoc to generate the language files at build time rather than checking them in. Just FYI :)

@mathieu-benoit
Copy link
Contributor

Thanks for sharing @mic-max, much appreciated!

@NimJay
Copy link
Collaborator Author

NimJay commented Jun 7, 2022

Wow, @mic-max, thank you so much for sharing your work!

I realized that the pull-requests (PRs) you shared contain changes (e.g., changes specific to the timestamp.proto that's not found in our* repo) that are outside the scope of this GitHub issue (#814), so we might have to do some careful sifting to leverage the PRs.
But we'll definitely take inspiration from them (and I'm sure we'll find them useful).
And, of course, feel free to create PRs into our* repo (containing just the .proto file relocations).

"our* repo" = github.com/GoogleCloudPlatform/microservices-demo

@NimJay
Copy link
Collaborator Author

NimJay commented Dec 7, 2022

Commenting to cool this issue down (from our team's out-of-SLO list):
We should still address this issue. I'll leave it open.

@bourgeoisor
Copy link
Member

Cooling down SLO-- Still in backlog.

@NimJay
Copy link
Collaborator Author

NimJay commented Dec 4, 2023

Team out-of-SLO cooldown: no progress on this issue.
This work would be useful — I'll leave this open.

@bourgeoisor bourgeoisor changed the title Tidy Up (and/or Document) .proto Files Tidy up (and document) .proto files Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/grpc priority: p3 Desirable enhancement or fix. May not be included in next release. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

4 participants