Add Protobuf support #41
Conversation
protobuf/sampling.proto
Outdated
| int32 max_traces_per_seconds = 1; | ||
| } | ||
|
|
||
| message SamplingStrategy { |
There was a problem hiding this comment.
I think this isn't a top level type, make sure you do proper nesting to minimize visibility
protobuf/sampling.proto
Outdated
| } | ||
|
|
||
| service SamplingManager { | ||
| rpc GetSamplingStrategy (ServiceRequest) returns (SamplingStrategyResponse); |
There was a problem hiding this comment.
with grpc it would make sense to use streaming for these comms
There was a problem hiding this comment.
So I was wondering in these types of cases where the request/response cycle is periodic but not exactly streaming. I guess it still works as long as we know we call it repeatedly.
protobuf/jaeger.proto
Outdated
|
|
||
| message Empty {}; | ||
|
|
||
| message ServiceRequest { |
There was a problem hiding this comment.
can we keep this cleaner that thrift by separating data model from service definitions?
There was a problem hiding this comment.
Sure. I would really call this common.proto. oneway isn't supported in gRPC (grpc/grpc#2147), so need a way to return nothing. Regarding ServiceRequest, Protobuf does not support request or response types that are not defined as message types beforehand, so no rpc Request(string service_name) returns (Response). Had to come up with this to wrap the string in two services (see baggage.proto and sampling.proto).
protobuf/jaeger.proto
Outdated
| } | ||
| Type type = 1; | ||
| uint64 trace_id_high = 2; | ||
| uint64 trace_id_low = 3; |
There was a problem hiding this comment.
I think I would prefer these two fields to be encapsulated into a TradeId struct, however there is a question of efficiency in the generated Go types. For example, Thrift would define traceId field as a pointer in such case, which requires an additional allocation for no reason.
This is actually a more general question. Before committing the IDLs, we should really run perf tests to compare thrift/tchannel vs. proto/grpc, in terms of throughput, latency, memory, CPU.
There was a problem hiding this comment.
Interesting question. For sure agree about the benchmarking. Actually started this change after reading about the different encoding/decoding benchmarks where it seems Protobuf has the best performance in Go, and presumably elsewhere. To set the record straight, when it comes to Go, there is the "fast" generator gogo/protobuf vs. the standard generator golang/protobuf. The reputation for speed is only true for the fast generator that I use throughout.
protobuf/sampling.proto
Outdated
| int32 max_traces_per_seconds = 1; | ||
| } | ||
|
|
||
| message OperationSamplingStrategy { |
There was a problem hiding this comment.
this can be embedded in PerOperationSamplingStrategy, correct?
There was a problem hiding this comment.
Guess so. I just copied this from Thrift. Then again, I suppose Thrift didn't support nesting.
protobuf/sampling.proto
Outdated
| double default_sampling_probability = 1; | ||
| double default_lower_bound_traces_per_second = 2; | ||
| repeated OperationSamplingStrategy per_operation_strategy = 3; | ||
| double default_upper_bound_traces_per_second = 4; |
There was a problem hiding this comment.
nit: group with other primitive fields
There was a problem hiding this comment.
Actually this is one of the more interesting issues with Protobuf v3. I think in Thrift, the defaultUpperBoundTracesPerSecond field is the only one marked as optional. Protobuf v3 makes all fields optional, so it makes sense. This default optional behavior may affect how we handle zero values.
protobuf/crossdock/tracetest.proto
Outdated
| syntax = "proto3"; | ||
| package jaegertracing.protobuf.crossdock; | ||
|
|
||
| enum Transport { |
There was a problem hiding this comment.
do you actually need this file right now? it's no longer an official Jaeger API, but a payload for crossdock tests, and those don't use proto/grpc. I would suggest leaving this out of this PR.
There was a problem hiding this comment.
No I don't. Just used it as an opportunity to port everything to Protobuf. Will remove for now.
|
Found out you can run |
Signed-off-by: Isaac Hier <isaachier@gmail.com>
Signed-off-by: Isaac Hier <isaachier@gmail.com>
Signed-off-by: Isaac Hier <isaachier@gmail.com>
Signed-off-by: Isaac Hier <isaachier@gmail.com>
Signed-off-by: Isaac Hier <isaachier@gmail.com>
Signed-off-by: Isaac Hier <ihier@uber.com>
Signed-off-by: Isaac Hier <isaachier@gmail.com>
|
Hello @yurishkuro What do we need to move this PR forward? I'd like to contribute on getting grpc integrated to Jaeger and this seems like one of the low hanging fruit. Cheers! |
|
@zkanda @isaachier this PR is premature, please see jaegertracing/jaeger#773 I think we should still keep this open since it contains useful build enhancements, and we can revisit it once some of the decisions are made in the above issue. |
|
replaced by #61 |
Decided to spend some time on this. Not sure if we should do this now, but I'd love to swap out Thrift/UDP in the C++ client library in favor of Protobuf/gRPC.