Support Zipkin v2 Span format #12
Conversation
There was a problem hiding this comment.
you've done very well just site reading! Here are some tests that might help you. Note the server code is more complex just because sometimes it receives spans that are merged from multiple hosts. In a tracer, it will be simpler as there's only one local endpoint per span:
| /** | ||
| * \brief Report the span is sharing between the client and the server. | ||
| */ | ||
| inline bool shared(void) const { return m_shared; } |
There was a problem hiding this comment.
so this would be set when you successfully extract a trace context from headers (and use it)
There was a problem hiding this comment.
set shared when extract span ID from context 5eb34ed
src/Span.h
Outdated
| writer.String(host->service_name()); | ||
|
|
||
| writer.Key("ipv4"); |
There was a problem hiding this comment.
ok, it's a bug, fixed in a2b1401
src/Span.h
Outdated
| for (auto &annotation : m_span.annotations) | ||
| { | ||
| if (annotation.value == TraceKeys::CLIENT_SEND || annotation.value == TraceKeys::CLIENT_RECV) { |
There was a problem hiding this comment.
there's also messaging ones, too.. openzipkin/zipkin#1677
src/Span.h
Outdated
| local_endpoint.reset(new Endpoint(annotation.host)); | ||
| } | ||
| if (!remote_endpoint && annotation.value == TraceKeys::SERVER_ADDR && |
There was a problem hiding this comment.
SERVER_ADDR is actually the remote side of span.kind=CLIENT
src/Span.h
Outdated
| for (auto &annotation : m_span.binary_annotations) | ||
| { | ||
| if (!local_endpoint && annotation.value == TraceKeys::CLIENT_ADDR && |
There was a problem hiding this comment.
CLIENT_ADDR is actually the remote side of span.kind=SERVER
There was a problem hiding this comment.
I have a little confuse :), c6c4723
src/Span.h
Outdated
| (annotation.host.__isset.ipv4 || annotation.host.__isset.ipv6)) { | ||
| remote_endpoint.reset(new Endpoint(annotation.host)); | ||
| } |
test/TestSpan.cpp
Outdated
| "parentId": "%016llx", | ||
| "kind": "CLIENT", | ||
| "timestamp": %lld, | ||
| "annotations": [ |
There was a problem hiding this comment.
to map span 1 format to span 2 involves a little more logic, particularly to remove redundancy (ex cs is the same as span.kind = client + start timestamp). The following is a little more complex than you need because from your tracer, you will only have one host in a span:
If you expose a primary recording api in span2 style, like brave's the translation to span1 model is easier
test/TestSpan.cpp
Outdated
| } | ||
| ], | ||
| "tags": { | ||
| "sa": "8.8.8.8", |
There was a problem hiding this comment.
sa should translate into remote address
| } | ||
|
|
||
| const std::vector<Span2> Span2::from_span(const Span *span) { |
There was a problem hiding this comment.
heh wow! you actually ported over the conversion code :)
There was a problem hiding this comment.
yes, it's too complicated to do it when serialize Span
| enum Kind { | ||
| UNKNOWN, | ||
| CLIENT, |
There was a problem hiding this comment.
PRODUCER and CONSUMER are also kinds, btw. sadly the conversion code got a bit longer to support these https://github.com/openzipkin/zipkin/blob/master/zipkin/src/main/java/zipkin/internal/Span2Converter.java#L300
There was a problem hiding this comment.
My question is whether the client library need to support those broker message types?
test/TestSpan.cpp
Outdated
| ASSERT_EQ(std::string(buffer.GetString(), buffer.GetSize()), std::string(str, str_len)); | ||
| } | ||
|
|
||
| TEST(span, serialize_json_v2) |
There was a problem hiding this comment.
probably want to add some more tests since there's a lot of conversion logic. Perhaps port some from here for the major cases https://github.com/openzipkin/zipkin/blob/master/zipkin/src/test/java/zipkin/internal/Span2ConverterTest.java
| struct Span2 { | ||
| enum Kind { | ||
| UNKNOWN, |
There was a problem hiding this comment.
we leave null for unknown vs an unknown constant. as long as you handle the same semantics up to you though
|
My question is whether the client library need to support those broker
message types
Well if you did, people could instrument c libraries correctly with your
library. For example, they could instrument a rabbit mq library and have
the correct constants available. You are right that for client/server it
isnt important.
|
| #@namespace scala com.twitter.zipkin.thriftscala | ||
| namespace rb Zipkin | ||
|
|
||
| struct DependencyLink { |
There was a problem hiding this comment.
don't think you'll need this in the tracer, mainly as you can't reliably create a link streaming (bc you don't know if downstream will be instrumented or not)
|
ps not required to implement the same way in C of course, but a late chat about java representation of the v2 format openzipkin/zipkin#1499 (comment) |
|
Great work @flier! any plan to continue this PR? |
resolve #11