Adds proto3 definition #47
Conversation
zipkin2.proto
Outdated
| } | ||
|
|
||
| message Annotation { | ||
| uint64 timestamp = 5; |
There was a problem hiding this comment.
Is the ordering here some sort of optimization? would be good to comment that if so
There was a problem hiding this comment.
heh this is a mistake!
|
Added some details about defaults due to gitter chat with @jorgheymans Also switched to fixed64 for timestamps. Epoch timestamp are encoded fixed64 are varint would also be 8 bytes, and more expensive to encode and size. Duration is stored uint64, as often the numbers are quite small. |
|
on the fixed64 for timestamps thing: one benefit is constant time for size calculation (returning 8 vs branching on the value) |
|
Love protobuff. 👍 |
This adds `Endpoint.ipv4Bytes()` and `Endpoint.ipv6Bytes()` needed to prevent excess overhead marshaling into byte arrays. It is efficient to do this once, as endpoint IPs are most often shared (local endpoint) for all spans. Even in the case of remote endpoint, these are often parsed from `Inet6Address` objects who already have a byte array reference. This will be used in proto3 and possibly in a future retrofitted v1 thrift encoder. See openzipkin/zipkin-api#47
|
openzipkin/zipkin#1998 shows incremental progress implementing this |
|
On port as int32, I think we should keep it. Here's some background. There is no uint16 type, so we can't be perfectly efficient with ports. For example, we can't encode high ports (those above 16383) in 2 bytes, rather 3. It is common that client-side ports are high ports (ex remoteEndpoint for server spans). That said, low ports like 80 take only 1 byte as varints smaller than 7 bits (127) are 1 byte to encode. Finally, there's really nothing we can do with this in proto3 Anyway, I found 3 other protos who use int32 for port, including grpc, so at least this has repeated: |
|
@openzipkin/core FYI I am very confident in the shape of this at the moment. There may be details and obviously will lift the docs from the openapi spec, but please do chime in if you see any problems |
|
LGTM |
|
Maybe add comments to expected usage of DependencyLink?
in fact, I'll remove it. it isn't necessary and we have no upload api for
them.
|
zipkin2.proto
Outdated
| Kind kind = 4; | ||
| string name = 5; | ||
| fixed64 timestamp = 6; | ||
| uint64 duration = 7; |
There was a problem hiding this comment.
What are the units for timestamp and duration?
The zipkin-api project has no text to define what a span is. This clarifies text so we can use it consistently. See openzipkin/zipkin-api#47
|
I've migrated docs from the yaml here. All ready to go in my opinion |
…2013) The zipkin-api project has no text to define what a span is. This clarifies text so we can use it consistently. See openzipkin/zipkin-api#47
…penzipkin#2013) The zipkin-api project has no text to define what a span is. This clarifies text so we can use it consistently. See openzipkin/zipkin-api#47
Design notes:
This is just like json, except uses bytes for IDs and IPs and enum instead of string for span kind
When finished will fix #39