Add Proto3 service definition

ewhauser commented 5 years ago

Now that we have a proto3 definition for Spans, we should define a service definition for them so that a GRPC server instance could be created. To start, the service definition could just be limited to the equivalent of the /spans endpoint in the current Open API spec.

codefromthecrypt commented 5 years ago

hmm I'm not entirely sure we want to inherit grpc responsibilities just because we have a proto encoding.

codefromthecrypt commented 5 years ago

so mainly I suggest we tread lightly as if we define something here, it is almost certain people will expect client and server support in all languages. It would be unintuitive to host a definition for something we don't support in other words.

If folks do want to support gRPC, though personally I hesistate from a library conflict POV... I think that discussion should happen. cc'ing @openzipkin/core for other opinions.

Meanwhile if users (possibly folks like @anuraaga or @huydx) want this, click thumbs up or otherwise. I suspect since few are watching this repo it actually might be better to open on openzipkin/zipkin...

ewhauser commented 5 years ago

So I’m specifically suggesting that we tread lightly here and limit the scope to a single endpoint capable of accepting a batch of spans.

I believe that Service definitions are not coupled to GRPC and are part of the raw proto3 spec. Thus, there is no dependency issue. (Need to verify)

I agree that you probably shouldn’t accept a PR against Zipkin server for a GRPC implementation, but I don’t think the Service definition implies support for an implementation.

codefromthecrypt commented 5 years ago

@ewhauser ok thanks.. please get back with what you find!

basvanbeek commented 5 years ago

Even though a service definition is not coupled to GRPC (it is indeed part of proto3, see: https://developers.google.com/protocol-buffers/docs/proto3#services). I still feel like we'd be not only defining what the service would look like but also imply we have some form of support in our ecosystem.

So i'd like to defer specifying a service before we actually have plans for supporting at least one implementation.

ewhauser commented 5 years ago

It's definitely part of the spec; what I wasn't sure if was whether it would force you to have a library dependency in any of the existing Zipkin codebase. GRPC is a compiler plugin for protoc so I don't see anywhere that would be the case.

I think it's pretty reasonable to assume that someone will want implementation support for this in the Zipkin ecosystem at some point. Writing implementations for sending spans via HTTP in every language is a losing battle; having a GRPC server implementation is a much better way to go. However, given that no one was opened a PR for that yet, I understand (and respect) the hesitation for not wanting to add this to the spec. It's a chicken or egg problem though - adding the definition to the spec would make it easier for others to experiment with this area. For clarity, we are about a definition like:

 service SpanService {
   rpc Publish (PublishSpansRequest) returns (PublishSpansResponse);
 }

 message PublishSpansRequest {
   ListOfSpans spans = 1;
 }

 message PublishSpansResponse {
 }

The exact details on the implementation could be debated in the PR, but it is not going to look much different than this.

Oddly enough, in my case, my initial interest for this is is defining a service boundary between a hybrid app and native code within a mobile application - not for providing a GRPC server. That is where the inspiration for adding it to the spec came from.

anuraaga commented 5 years ago

Since I was added I'll just add my opinion that zipkin having an official gRPC API sounds like a really great idea. I also agree that defining the proto without committing to making an official implementation does seem incomplete.

ewhauser commented 5 years ago

I experimented with adding a gRPC server implementation to zipkin-server. It doesn't appear to be a challenge from a dependency perspective. The gRPC server implementation can be supported without adding bloat much like the optional storage collectors:

https://github.com/ewhauser/zipkin/tree/experimental-grpc

A couple of things were learned:

There would not be any additional dependencies in zipkin-server by adding a Service methods to the top level API. zipkin-server doesn't have a dependency here except for its benchmarks
If it took one later, this would also not introduce a dependency on gRPC
There really isn't a lot of value in adding the service definitions if there is no interest in supporting a server implementation

If there is interest, I can open a PR against zipkin-server for discussion. If we wanted to make it really clear that this was experimental, unsupported ATM then it could be its own repo.

codefromthecrypt commented 5 years ago

hi eric. grpc has a number of toxic deps (guice, guava, netty). the only reason why it doesn't conflict now is because we moved everything we could out of the way (so that stackdriver could work)

if we change cassandra for example to the latest version of the driver, it will play ping-pong with netty with grpc. if we do add grpc we need to ensure stackdriver uses exactly the same version, which is ok for now because we made our own driver for it.. this could break easily though as at some point the versions of things we use could conflict with the auth libraries. https://github.com/openzipkin/zipkin-gcp/tree/master/autoconfigure

just noting that the reason you see no conflicts now is that we had to rejig everything already :P

codefromthecrypt commented 5 years ago

PS I'm not suggesting we rule out this, just if we do it, we should...

test with the new cassandra 4 driver (which will be out soonish)

make stackdriver (in our zipkin-gcp) choose explicitly a compatible version of boringssl until such time as our base image can move to JRE11+ which it can't right now as the alpine is being blocked and alternatives are 4x the size. this is not a problem per-se, just a consideration. We already have to align things like spring boot versions.

make sure stackdriver (in our zipkin-gcp) has a compatible future for the auth libraries (also unlikely to be a problem except coordination)

with homework like above in, I can get behind this cc @anuraaga

codefromthecrypt commented 5 years ago

so TL;DR; is if we add anything in the main repo, it ends up in the exec jar for the server, which has knock-on effects for classpath in our satellite repos. We need to watch out to make sure we don't make zipkin-gcp broke.

consider zipkin grpc listener forwarding to stackdriver in other words. both will need to agree on the grpc version, and versions of their dependencies.

ewhauser commented 5 years ago

Acknowledge the concerns. This ends up being a bigger support issue for the project at whole than the submitter.

I think most of these can be worked around. Here are the dependencies that this would add:

[INFO] io.zipkin.zipkin2:zipkin-server-grpc:jar:2.11.13-SNAPSHOT
[INFO] +- io.zipkin.zipkin2:zipkin:jar:2.11.13-SNAPSHOT:compile
[INFO] +- io.zipkin.proto3:zipkin-proto3:jar:0.1.0:compile
[INFO] +- io.zipkin.zipkin2:zipkin-collector:jar:2.11.13-SNAPSHOT:compile
[INFO] +- io.grpc:grpc-netty-shaded:jar:1.17.1:runtime
[INFO] |  \- io.grpc:grpc-core:jar:1.17.1:compile (version selected from constraint [1.17.1,1.17.1])
[INFO] |     +- io.grpc:grpc-context:jar:1.17.1:compile
[INFO] |     +- com.google.code.gson:gson:jar:2.7:compile
[INFO] |     +- com.google.errorprone:error_prone_annotations:jar:2.2.0:compile
[INFO] |     +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
[INFO] |     +- org.codehaus.mojo:animal-sniffer-annotations:jar:1.17:compile
[INFO] |     +- io.opencensus:opencensus-api:jar:0.17.0:compile
[INFO] |     \- io.opencensus:opencensus-contrib-grpc-metrics:jar:0.17.0:compile
[INFO] +- io.grpc:grpc-protobuf:jar:1.17.1:compile
[INFO] |  +- com.google.protobuf:protobuf-java:jar:3.5.1:compile
[INFO] |  +- com.google.guava:guava:jar:26.0-android:compile
[INFO] |  |  +- org.checkerframework:checker-compat-qual:jar:2.5.2:compile
[INFO] |  |  \- com.google.j2objc:j2objc-annotations:jar:1.1:compile
[INFO] |  +- com.google.api.grpc:proto-google-common-protos:jar:1.0.0:compile
[INFO] |  \- io.grpc:grpc-protobuf-lite:jar:1.17.1:compile
[INFO] +- io.grpc:grpc-stub:jar:1.17.1:compile

To avoid any issues with netty, I'm using grpc-netty-shaded
boringssl is an optional dependency so we should have control over which version we use. The GCP Stackdriver client is using the latest version of gRPC so these should be compatible (I can integration test)
It looks like the would be if Cassandra is using an incompatible version of guava which is as much a Cassandra issue as anything else

ewhauser commented 5 years ago

I'll shore this up a little bit and open a PR in zipkin where we can continue the discussion.

codefromthecrypt commented 5 years ago

https://github.com/openzipkin/zipkin-api/pull/59

openzipkin / zipkin-api

Add Proto3 service definition #57