Add support to Zipkin V1 #271
Conversation
|
This PR is too large - maybe create a separate PR to check in the test data first? |
|
@songy23 The test data came straight from Envoy but there was a bit of redundant stuff in terms of code coverage. Removed redundant parts keeping the core annotations to document the type of annotation that we expect to get. That is about 550 less lines of code on the tests. Unfortunately, expressing the OC proto in go is a bit verbose, we have other tests like that but perhaps it is a good idea to find a less verbose way to express them. There seems to be something wrong with the go dependencies on Travis but at first glance I don't think it was introduced by my change. |
| return nil | ||
| } | ||
|
|
||
| // Unknow service name works both as a default value and a flag to indicate that a valid endpoint was found. |
| res := &annotationParseResult{} | ||
| timeEvents := make([]*tracepb.Span_TimeEvent, 0, len(annotations)) | ||
| for _, currAnnotation := range annotations { | ||
| if currAnnotation == nil && currAnnotation.Value != "" { |
There was a problem hiding this comment.
This condition can't ever hold and will always panic if currAnnotation == nil. Did you mean
if currAnnotation == nil || curAnnotation.Value == "" {
continue
}| } | ||
| } | ||
|
|
||
| func sortTrace(trace []*agenttracepb.ExportTraceServiceRequest) { |
There was a problem hiding this comment.
Please update this name to sortTracesByName
receiver/zipkin/trace_receiver.go
Outdated
| @@ -110,13 +112,30 @@ func (zr *ZipkinReceiver) StartTraceReception(ctx context.Context, spanSink rece | |||
| return err | |||
| } | |||
|
|
|||
| func (zr *ZipkinReceiver) parseAndConvertToTraceSpans(blob []byte, hdr http.Header) (reqs []*agenttracepb.ExportTraceServiceRequest, err error) { | |||
| func (zr *ZipkinReceiver) parseAndConvertToTraceSpans(blob []byte, r *http.Request) (reqs []*agenttracepb.ExportTraceServiceRequest, err error) { | |||
There was a problem hiding this comment.
While I realize that this arguments change was made to keep the tests simple and easily reuse the code, but unfortunately I think that now we don't spell out that we handle both Zipkin v1 and v2. To make that clear, I think we can keep the contents of this function as it was before but rename it to v2ToTraceSpans and then make one v1ToTraceSpans while keeping the multiplexing logic at the ServeHTTP level.
so
// v1ToTraceSpans parses Zipkin v1 JSON traces and converts them to OpenCensus Proto spans.
func (zr *ZipkinReceiver) v1ToTraceSpans(blob []byte, hdr http.Header) (reqs []*agenttracepb.ExportTraceServiceRequest, err error) {
return tracetranslator.Zipkinv1JSONBatchToOCProto(blob)
}// v2ToTraceSpans parses Zipkin v2 JSON or Protobuf traces and converts them to OpenCensus Proto spans.
func (zr *ZipkinReceiver) v2ToTraceSpans(blob []byte, hdr http.Header) (reqs []*agenttracepb.ExportTraceServiceRequest, err error) {
...
}There was a problem hiding this comment.
Yeah, it will be better with two separate functions.
receiver/zipkin/trace_receiver.go
Outdated
| @@ -237,7 +256,7 @@ func (zr *ZipkinReceiver) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| _ = c.Close() | |||
| } | |||
| _ = r.Body.Close() | |||
| ereqs, err := zr.parseAndConvertToTraceSpans(slurp, r.Header) | |||
| ereqs, err := zr.parseAndConvertToTraceSpans(slurp, r) | |||
There was a problem hiding this comment.
Checking for r != nil is impossible because we already just dereferenced r, please keep this signature.
and then in here is where we'll do the multiplexing.
_ = r.Body.Close
// Now deserialize and process the spans.
const asZipkinv1 = r.URL != nil && strings.HasPrefix(r.URL.Path, "/api/v1/spans")
var ereqs []*agenttracepb.ExportTraceServiceRequest
var err error
if asZipkinv1 {
ereqs, err = zr.v1ToTraceSpans(slurp, r.Header)
} else {
ereqs, err = zr.v2ToTraceSpans(slurp, r.Header)
}
receiver/zipkin/proto_parse_test.go
Outdated
| @@ -102,9 +102,10 @@ func TestConvertSpansToTraceSpans_protobuf(t *testing.T) { | |||
| zi := new(ZipkinReceiver) | |||
| hdr := make(http.Header) | |||
| hdr.Set("Content-Type", "application/x-protobuf") | |||
| r := &http.Request{Header: hdr} | |||
There was a problem hiding this comment.
If we keep the multiplexing in ServeHTTP while cleanly separating the methods for v1 and v2, then we don't have to manually create this Request or make this line change and make a specific unit test for v1.
If we want to ensure that they properly multiplex, we can just use ServeHTTP in tests like this
req := http.NewRequest("POST", "http://opencensus.io/", blob)
req.Header.Set("Content-Type", CONTENT_TYPE)
rec := httptest.Recorder()
zi.ServeHTTP(rec, req)
res := rec.Result()
zBlob, _ := ioutil.ReadAll(res.Body)
zreqs, err := json.Unmarshal(zBlob, &recv)Before removing dead code Clean-up and multiple batches test Add Node attributes and some cleanup Remove unused file
The test data came straight from Envoy but there was a bit of redundant stuff in terms of code coverage. Removed most of it keeping the core annotations to document the type of annotation that we expect to get.
ce699c3 to
54df561
Compare
|
Thanks for the review @odeke-em, code looks better now 👍 |
* Add support to Zipkin V1 * Reducing test data without affecting coverage. * PR Feedback
Fixes #250
Add a trace translator from Zipkin V1 to OC proto.
Per ealier dicussion we just need to support JSON,
this gets OC service working with current Envoy tracing
for Zipkin.