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

Remove dependency on github.com/grpc-ecosystem/grpc-gateway #2843

Closed
yurishkuro opened this issue Feb 25, 2021 · 6 comments · Fixed by #2858
Closed

Remove dependency on github.com/grpc-ecosystem/grpc-gateway #2843

yurishkuro opened this issue Feb 25, 2021 · 6 comments · Fixed by #2858
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

As far as I can tell, we are not using it today. The motivation for it was to have a canonical proto-based JSON API that would be used by the UI as well as a public API. But there are issues with proto-based JSON (e.g. IDs are represented in base64 instead of hex)
and updating the UI is a large task that we don't have the cycles for. I suggest we remove this dependency altogether until the time we actually need it.

$ grx grpc-gateway .
./idl/proto/api_v2/sampling.proto:34:// Enable registration with golang/protobuf for the grpc-gateway.
./idl/proto/api_v2/query.proto:37:// Enable registration with golang/protobuf for the grpc-gateway.
./idl/proto/api_v2/collector.proto:35:// Enable registration with golang/protobuf for the grpc-gateway.
./idl/proto/api_v2/model.proto:38:// Enable registration with golang/protobuf for the grpc-gateway.
./go.mod:35:	github.com/grpc-ecosystem/grpc-gateway v1.14.5
./Makefile:476:	# Generate gogo, gRPC-Gateway, swagger, go-validators, gRPC-storage-plugin output.
./Makefile:484:	# --grpc-gateway_out generates gRPC-Gateway output.
./Makefile:485:	# --swagger_out generates an OpenAPI 2.0 specification for our gRPC-Gateway endpoints.
./Makefile:506:		### grpc-gateway generates 'query.pb.gw.go' that does not respect (gogoproto.customname) = "TraceID"
./Makefile:507:		### --grpc-gateway_out=$(PROTO_GOGO_MAPPINGS):$(PWD)/proto-gen/ \
./proto-gen/api_v2/sampling.pb.go:14:	_ "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/options"
./proto-gen/api_v2/query.pb.go:15:	_ "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/options"
./proto-gen/api_v2/collector.pb.go:13:	_ "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/options"
./plugin/storage/grpc/proto/storage.proto:34:// Enable registration with golang/protobuf for the grpc-gateway.
./tools.go:23:	_ "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger"
./go.sum:313:github.com/grpc-ecosystem/grpc-gateway v1.9.0/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY=
./go.sum:314:github.com/grpc-ecosystem/grpc-gateway v1.9.5/go.mod h1:vNeuVxBJEsws4ogUvrchl83t/GYV9WGTSLVdBhOQFDY=
./go.sum:315:github.com/grpc-ecosystem/grpc-gateway v1.14.5 h1:aiLxiiVzAXb7wb3lAmubA69IokWOoUNe+E7TdGKh8yw=
./go.sum:316:github.com/grpc-ecosystem/grpc-gateway v1.14.5/go.mod h1:UJ0EZAp832vCd54Wev9N1BMKEyvcZ5+IM0AwDrnlkEc=
@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Feb 25, 2021
@jeffy-mathew
Copy link
Contributor

jeffy-mathew commented Feb 26, 2021

Hi, I would like to work on this issue

@yurishkuro
Copy link
Member Author

@jeffy-mathew it's yours

@jeffy-mathew
Copy link
Contributor

@yurishkuro I need some help in generating proto files, how to remove the dependency from generated proto files?

@yurishkuro
Copy link
Member Author

oh, you may need to make a change to jaeger-idl repo and remove the respective annotations from these files:

./idl/proto/api_v2/sampling.proto
./idl/proto/api_v2/query.proto
./idl/proto/api_v2/collector.proto
./idl/proto/api_v2/model.proto

@jeffy-mathew
Copy link
Contributor

about the changes in submodule jaeger-idl, do I need to raise a separate PR in that repo?

@yurishkuro
Copy link
Member Author

Yes, a PR there and after it's merged update the submodule in this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants