-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add proto3 encoding #1942
Comments
Looking at docs and generated code in proto3, I think the following are true for our key functions. Note that I'm assuming zipkin will use a field number less than 128 for the span data (StackDriver is field number 1, so yeah works for them too).
@basvanbeek this seem to gel with your understanding of proto3? It looks like what the generated code does: size = 0;
for (int i = 0; i < traces_.size(); i++) {
size += com.google.protobuf.CodedOutputStream
.computeMessageSize(1, traces_.get(i));
} https://developers.google.com/protocol-buffers/docs/encoding |
FYI @saturnism I will try to get this in by zipkin 2.6 which should land sometime in the next week or so depending on the UI-related changes that are still in flight. After that, it should be possible to make the zipkin-gcp sender using stackdriver-specific proto3 encoding even if we haven't defined a proto3 zipkin fomat yet |
will this eventually lead to zipkin starting as a grpc server ? or it's really just the protobuf encoding of the traces at this point ? |
will this eventually lead to zipkin starting as a grpc server ? or it's
really just the protobuf encoding of the traces at this point ?
no. this is just data format. similar to how we handled thrift, to the
degree we have core lib support for grpc, we'd do it in a zero dependency
way. gRPC is a somewhat toxic dependency already, for example, if we
packaged anything we'd have a wrestling match with zipkin-grpc for the
version of boringssl + grpc etc. This is more about making the data format
possible, and later defined.
|
s/to the degree we have core lib support for grpc/to the degree we have
core lib support for proto3/
sorry!
|
While we haven't yet defined a proto3 format, this allows one to be defined. This also allows us to refine how stackdriver (or any future translated format) works in zipkin reporter. Implementation is relatively simple: this assumes a buffer of spans is analogous to a proto3 repeated field, and that the field name itself is ordinal <=127. Fixes #1942
While we haven't yet defined a proto3 format, this allows one to be defined. This also allows us to refine how stackdriver (or any future translated format) works in zipkin reporter. Implementation is relatively simple: this assumes a buffer of spans is analogous to a proto3 repeated field, and that the field name itself is ordinal <=127. Fixes #1942
While we haven't yet defined a proto3 format, this allows one to be defined. This also allows us to refine how stackdriver (or any future translated format) works in zipkin reporter. Implementation is relatively simple: this assumes a buffer of spans is analogous to a proto3 repeated field, and that the field name itself is ordinal <=127. Fixes openzipkin#1942
We should eventually map out a proto3 encoding even if this is just a direct translation from the json. We don't yet have an issue for this, but it is in response to #1644 and its parent.
In the mean time, we should at least make it possible in the java library to do this. Minimally, this requires adding zipkin2.codec.Encoding.PROTO3 to permit this format when using the async reporter.
The text was updated successfully, but these errors were encountered: