Define project layout for Protobuf / gRPC / generated files

[yurishkuro]

As part of #773, we want to separate model definition and services API into different .proto files. There are several decisions that we need to make:

1. Single or multiple files for definitions of different services (collector vs. query vs. config/sampling)
2. The name of protobuf package
3. Go package for the API definitions
4. Go package for implementations

Proposal

Based on test implementation in #903.

Multiple .proto files

The model.proto should be separate because we want to aim it at the model package in the backend, which is not suitable for the APIs.

The services files can be either separate like collector.proto, query.proto, or combined into a single file like api_v2.proto. I recommend the latter for now, for simplicity. It also results in a single swagger file, which would otherwise be split into multiple files

The name of protobuf package

My WIP is using package jaeger.api_v2;

Go package for the API definitions

proto-gen/api_v2, except for model.proto which is generated under model directly. Once we move all proto files to jaeger-idl repository we may want to change go_package in model.proto to api_v2 as well, so that when people generate classes for client libs they end up with everything under the same package name. In the backend repo we can rewrite that definition to model before generating code.

Go package for implementations

This one is tricky. Currently we separate implementations by the respective services that contain them, i.e. collector service is only implemented in cmd/collector, same for sampling / throttling service, while query service is in cmd/query. This is mostly an implementation detail that we can change later, so I would go with similar structure using api_v2 package, e.g. cmd/collector/app/api_v2.

Open Questions

• Are we happy with api_v2? I started with api.v2, but found it to be clunky, esp. in Go where the default package name is the last segment (so v2).
• If someone has experience with hosting swagger UI, does it support multiple JSON descriptors?
• If someone has experience with using Protobuf in multiple languages, does any of the above sound completely bonkus?

[jpkrohling]

The services files can be either separate like collector.proto, query.proto, or combined into a single file like api_v2.proto. I recommend the latter for now, for simplicity. It also results in a single swagger file, which would otherwise be split into multiple files

What's easier to consume, for someone on the outside? If it's easier to build a client sending data directly to the collector by following the Swagger output for collector.proto, then that would be my preference, even if it requires (reasonably) more work from us.

Are we happy with api_v2? I started with api.v2, but found it to be clunky, esp. in Go where the default package name is the last segment (so v2).

What's more usual in Go? I've seen the api.v2 notation quite often.

[yurishkuro]

If you can add the links to where you saw v2 for proto I can investigate more.

[jpkrohling]

v2 for proto

Sorry, I mean as Go imports.

[yurishkuro]

Envoy uses v2 as the package name: https://github.com/envoyproxy/data-plane-api/blob/8e13c8b4af1e78836fcf51f94fda7b63992c4e6c/envoy/api/v2/discovery.proto#L3-L6. Also vgo typically recommends similar packaging, e.g. import "my/thing/v2/sub/pkg.

What's easier to consume, for someone on the outside?

If we generate multiple swagger files, then each of them will repeat the definition of the model types. So if you wanted to build an external JSON client that depends on both collector and query services, and you used separate swagger files to generate the code, you'd end up with two copies of the model classes, unless you tweak something. On that basis I think a single swagger file is easier to consume. But for consuming proto files, I don't think it matters if they are split or grouped.

[jpkrohling]

you'd end up with two copies of the model classes, unless you tweak something

If that's the case, then it's clear that we do want a single file :)

[Falco20019]

If we generate multiple swagger files, then each of them will repeat the definition of the model types. So if you wanted to build an external JSON client that depends on both collector and query services, and you used separate swagger files to generate the code, you'd end up with two copies of the model classes, unless you tweak something.

Haven't looked into swagger with protos. Does it support import of protos or is it still duplicating model types? Because normally you would do a proto file for shared types and just include it.

If you can add the links to where you saw v2 for proto I can investigate more.

At https://github.com/grpc/grpc/blob/master/src/proto/grpc/lb/v1/load_balancer.proto is an example directly from Google.

[annanay25]

Hi,

Just bumping the discussion on this, as we proceed on #1307, should we separate .proto files?

[yurishkuro]

Yes, we want to separate services files.

[annanay25]

About the discussion on multiple swagger files, protoc-gen-swagger now has support for generating a single swagger file by merging multiple .proto files -

https://github.com/grpc-ecosystem/grpc-gateway/blob/0c2b3b165f486a2c49b627e71880172d27607a04/protoc-gen-swagger/main.go#L22

---

So I managed to separate out query-service definitions from the api_v2.proto file and generate a unified swagger file -
annanay25@0ee8357

... took a while (and some makefiles) to figure out passing flags to protoc/protoc-gen-swagger (which doesn't have great documentation unfortunately). Placing it here for reference -

protoc -I/usr/local/include -I. -Imodel/proto -Ivendor/github.com/gogo/protobuf/ -Ivendor/github.com/grpc-ecosystem/grpc-gateway/ \
  -I$GOPATH/src \
  -I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
  --plugin=protoc-gen-swagger=~/Desktop/git/go//bin/protoc-gen-swagger --swagger_out=logtostderr=true,allow_merge=true:. \
  model/proto/api_v2.proto model/proto/query.proto

[annanay25]

@yurishkuro could you let me know if this works for us?

[yurishkuro]

Yes it looks fine to me.

[annanay25]

Just a nit, should we go with -

model/proto/query.proto
model/proto/collector.proto
.
.

or

model/proto/api_v2/query.proto
model/proto/api_v2/collector.proto
.
.

[yurishkuro]

the latter (api_v2)

[yurishkuro]

Addressed by #1427