Zipkin json encoding via HTTP #334

pavolloffay · 2017-08-15T04:41:52Z

Fixes https://github.com/uber/jaeger/issues/225

Zipkin API definition: https://github.com/openzipkin/zipkin-api/blob/master/zipkin-api.yaml

Docker Image to try it: https://hub.docker.com/r/pavolloffay/tmp-jaeger-zipkin-json/

coveralls · 2017-08-15T04:52:08Z

Coverage remained the same at 100.0% when pulling a152d1d on pavolloffay:zipkin-json into db9f152 on uber:master.

pavolloffay · 2017-08-15T04:58:44Z

How are we going to write integration tests for this? zipkin-go-opentracing supports only thrift. We could use brave but it's in java. I think we have two options:

modify xdock-java to use brave
crate new xdock-zipkin which would verify zipkin related stuff.

yurishkuro

I didn't do a completely thorough review, had a few starter questions. Overall direction looks good.

cc @black-adder

yurishkuro · 2017-08-15T04:44:11Z

cmd/collector/app/zipkin/http_handler.go

+		if err != nil {
+			http.Error(w, fmt.Sprintf(app.UnableToReadBodyErrFormat, err), http.StatusBadRequest)
+			return
+		}
 	} else {
 		http.Error(w, "Only Content-Type:application/x-thrift is supported at the moment", http.StatusBadRequest)


extend err msg

The comment is odd, but why custom error? I don't see a value for it

I meant err msg says only x-thrift is supported, not json, so we can extend it to mention both

yurishkuro · 2017-08-15T04:49:48Z

cmd/collector/app/zipkin/http_handler_test.go

@@ -148,7 +187,7 @@ func TestGzipBadBody(t *testing.T) {
 func TestUnsupportedContentType(t *testing.T) {
 	server, _ := initializeTestServer(nil)
 	defer server.Close()
-	statusCode, _, err := postBytes(server.URL+`/api/v1/spans`, []byte{}, createHeader("application/json"))
+	statusCode, _, err := postBytes(server.URL+`/api/v1/spans`, []byte{}, createHeader("text/html"))


xml would be funnier

video/mp4 :P

yurishkuro · 2017-08-15T04:51:50Z

cmd/collector/app/zipkin/zipkin_json.go

+type binaryAnnotation struct {
+	Endpoint endpoint `json:"endpoint"`
+	Key      string   `json:"key"`
+	Value    string   `json:"value"`


why did Adrian refer to oneOf? Is this the right JSON format?

I think oneOf refers to bool, string num so on. In the current https://github.com/openzipkin/zipkin-api/blob/master/zipkin-api.yaml it's defined as string

yurishkuro · 2017-08-15T04:53:15Z

cmd/collector/app/zipkin/zipkin_json.go

+	}
+
+	for _, span := range spans {
+		if len(span.TraceID) != 16 {


how are we dealing with 128bit IDs? Does Zipkin JSON encode them as one string, or hi/lo?

a good point, It's hi/low. New thrift it supports https://github.com/openzipkin/zipkin-api/blob/master/thrift/zipkinCore.thrift#L493. For now we could do only one part and once #335 is merged do both.

https://github.com/openzipkin/zipkin/blob/cf4df43952f2b82496c2e4c730765e7b70eba22b/zipkin/src/main/java/zipkin/internal/Util.java#L114
https://github.com/openzipkin/zipkin/blob/ba025306985da1f253665ac053d290a0f21f841c/zipkin/src/main/java/zipkin/internal/JsonCodec.java#L348-L351

Currently parsing only traceId[16:] (https://github.com/openzipkin/zipkin/blob/cf4df43952f2b82496c2e4c730765e7b70eba22b/zipkin/src/main/java/zipkin/internal/Util.java#L105). Once new thrift is updated traceIdHigh will be set with [:16]

yurishkuro · 2017-08-15T04:55:24Z

cmd/collector/app/zipkin/zipkin_json.go

+		if len(span.ID) != 16 && len(span.ID) != 32 {
+			return nil, errSpanID
+		}
+		if len(span.ParentID) > 0 && len(span.ParentID) != 16 {


I never understood this restriction in Zipkin to have the exact length - what if some leading zeroes were dropped for whatever reason, we can still parse the hex string. Especially for parsing out-of-band span data, I am not sure why we need to be this strict.

Honestly, I don't know, it's defined in api definition.

@adriancole hi, would you give us some insights here?

@yurishkuro updated to align with your comment. It seems that zipkin sever does the same.

yurishkuro · 2017-08-15T04:59:11Z

cmd/collector/app/zipkin/zipkin_json.go

+	}, nil
+}
+
+func parseIpv4(str string) (int32, error) {


so we're only parsing IPv4 from Zipkin, not IPv6?

Note that our zipkin.thrift (in jaeger-idl) is probably behind the official Zipkin IDL, e.g. we don't have IPv6 or TraceIdHigh fields.

yes, the reason is that our zipkin.thrift does not support ipv6. I will leave TODO comment there.

Yeah, we should sync it up with official IDL (should be backwards compatible) - #335

yurishkuro · 2017-08-15T05:06:07Z

How are we going to write integration tests for this?

I think we might be able to use the same xdock driver, but a different "client" that's implemented with Zipkin's JSON and Thrift official senders (two clients, really)

@black-adder - can we come up with a unified end-to-end testing framework? Ultimately it boils down to two parts

running some configuration of the backend (could be all-in-one, or docker-compose, or k8s)
having the driver hit some sort of client that can report spans to the backend (either via agent or via direct POST)

coveralls · 2017-08-15T06:07:48Z

Coverage remained the same at 100.0% when pulling 990f499 on pavolloffay:zipkin-json into 79fba12 on uber:master.

coveralls · 2017-08-15T09:40:41Z

Coverage remained the same at 100.0% when pulling 6a83968 on pavolloffay:zipkin-json into 79fba12 on uber:master.

black-adder · 2017-08-15T14:49:08Z

cmd/collector/app/zipkin/zipkin_json.go

+}
+
+var (
+	errIstUnsignedLog = errors.New("id is not an unsigned long")


nit: s/ist/is

black-adder · 2017-08-15T14:58:24Z

cmd/collector/app/zipkin/http_handler_test.go

@@ -122,6 +122,45 @@ func TestThriftFormat(t *testing.T) {
 	assert.EqualValues(t, "Cannot submit Zipkin batch: Bad times ahead\n", resBodyStr)
 }

+func TestJsonFormat(t *testing.T) {


nit: could we use a fixture for this test instead which defines the test cases and then run the cases at once?

black-adder · 2017-08-15T14:59:03Z

cmd/collector/app/zipkin/http_handler.go

 	} else {
-		http.Error(w, "Only Content-Type:application/x-thrift is supported at the moment", http.StatusBadRequest)
+		http.Error(w, "Not supported Content-Type", http.StatusBadRequest)


nit: s/Not Supported/Unsupported/ we can literally save bytes

black-adder · 2017-08-15T15:02:16Z

cmd/collector/app/zipkin/zipkin_json.go

@@ -0,0 +1,234 @@
+// Copyright (c) 2017 Uber Technologies, Inc.


file can be json.go since it's already namespaced under zipkin.

black-adder · 2017-08-15T15:18:24Z

cmd/collector/app/zipkin/zipkin_json.go

+}
+
+func decode(body []byte) ([]zipkinSpan, error) {
+	type zipkinSpans []zipkinSpan


is this intermediate type definition necessary?

it can be defined in global scope, is that better?

black-adder · 2017-08-15T15:25:59Z

cmd/collector/app/zipkin/zipkin_json.go

+	return tSpan, nil
+}
+
+func endpointToThrift(endp endpoint) (*zipkincore.Endpoint, error) {


go prefers incomprehensible variable names for short functions: https://talks.golang.org/2014/names.slide#7

so here s/endp/e

incomprehensible 😃 then I will rename all local vars to i

black-adder · 2017-08-15T15:27:47Z

cmd/collector/app/zipkin/zipkin_json_test.go

+import (
+	"encoding/json"
+	"fmt"
+	"github.com/stretchr/testify/assert"


import ordering

black-adder · 2017-08-15T15:29:10Z

cmd/collector/app/zipkin/zipkin_json_test.go

+	assert.Equal(t, "foo", endp.ServiceName)
+	assert.Equal(t, "127.0.0.1", endp.IPv4)
+	assert.Equal(t, "2001:db8::c001", endp.IPv6)
+	// only service name


this test and a couple of other tests might be slightly more readable with fixtures

black-adder · 2017-08-15T15:35:46Z

running some configuration of the backend (could be all-in-one, or docker-compose, or k8s)

having the driver hit some sort of client that can report spans to the backend (either via agent or via direct POST)

This can be easily done with in the all-in-one integration test, however doing it via crossdock tests does seem more semantically correct. The all-in-one integration test is supposed to be a simple test that ensures the pipeline works, I don't want to conflate the test with zipkin testing logic.

Creating a test zipkin client which sends http and thrift spans to the collectors and making this run with the xdock tests seems the best approach. But I'm not entirely sure how to get this to work without creating another docker image just for the zipkin client, I'll have to experiment a bit.

yurishkuro · 2017-08-15T16:04:08Z

btw, their new format with Go: https://github.com/openzipkin/zipkin-api-example/tree/master/go

coveralls · 2017-08-16T07:41:22Z

Coverage remained the same at 100.0% when pulling 2ada3b2 on pavolloffay:zipkin-json into 9bfab15 on uber:master.

coveralls · 2017-08-16T08:06:21Z

Coverage remained the same at 100.0% when pulling eafd168 on pavolloffay:zipkin-json into 9bfab15 on uber:master.

coveralls · 2017-08-16T09:03:16Z

Coverage remained the same at 100.0% when pulling 1879f5e on pavolloffay:zipkin-json into 9bfab15 on uber:master.

coveralls · 2017-08-16T11:31:04Z

Coverage remained the same at 100.0% when pulling cce519b on pavolloffay:zipkin-json into 5e5a361 on uber:master.

coveralls · 2017-08-16T12:26:32Z

Coverage remained the same at 100.0% when pulling 2603165 on pavolloffay:zipkin-json into 5e5a361 on uber:master.

coveralls · 2017-08-16T12:39:18Z

Coverage remained the same at 100.0% when pulling 64499c2 on pavolloffay:zipkin-json into 5e5a361 on uber:master.

coveralls · 2017-08-16T12:56:18Z

Coverage remained the same at 100.0% when pulling 2b406ae on pavolloffay:zipkin-json into 5e5a361 on uber:master.

pavolloffay · 2017-08-16T14:03:29Z

This is updated and should be ready for re-review.

black-adder · 2017-08-16T13:34:24Z

cmd/collector/app/zipkin/http_handler_test.go

+		expected   string
+		statusCode int
+	}{
+		{spanJSON, "Cannot submit Zipkin batch: Bad times ahead\n", http.StatusInternalServerError},


please explicitly define the variable names in the struct: ie {payload: spanJSON, etc.}

we've been bad about this in the past but should do it for everything going forward

black-adder · 2017-08-16T13:46:35Z

cmd/collector/app/zipkin/json.go

+	}
+
+	var result uint64
+	for i := start; i < len; i++ {


Could we use official the builtin golang functions for this: https://play.golang.org/p/0GUjRvy0kw or would it not work?

it produces different numbers

why not use model functions like model/TraceIDFromString?

https://github.com/uber/jaeger/blob/master/model/span.go#L146

It automatically takes care of high/low.

black-adder · 2017-08-16T13:49:26Z

cmd/collector/app/zipkin/json.go

+	}, nil
+}
+
+func parseIpv4(str string) (int32, error) {


can we make use of https://golang.org/pkg/net/#ParseIP and https://gist.github.com/ammario/649d4c0da650162efd404af23e25b86b ?

it did not work https://paste.fedoraproject.org/paste/mMkVJUks1K6pexIej3XDRA/ https://paste.fedoraproject.org/paste/DwfZ8v4DtYrLKjeb2bNbjw

ParseIP returns IP, which is an alias to []byte. If nil return 0 else pack bytes into long

coveralls · 2017-08-16T16:06:03Z

Coverage remained the same at 100.0% when pulling 3211e3d on pavolloffay:zipkin-json into 5e5a361 on uber:master.

yurishkuro · 2017-08-17T04:48:07Z

cmd/collector/app/zipkin/http_handler.go

 	} else {
-		http.Error(w, "Only Content-Type:application/x-thrift is supported at the moment", http.StatusBadRequest)
+		http.Error(w, "Unsupported Content-Type", http.StatusBadRequest)


missing return?

yurishkuro · 2017-08-17T04:48:40Z

cmd/collector/app/zipkin/http_handler.go

+	var tSpans []*zipkincore.Span
+	if contentType == "application/x-thrift" {
+		tSpans, err = deserializeThrift(bodyBytes)
+		if err != nil {


can do this outside of the if contentType==

yurishkuro · 2017-08-17T04:51:28Z

cmd/collector/app/zipkin/http_handler_test.go

+		expected   string
+		statusCode int
+	}{
+		{payload: spanJSON, expected: "Cannot submit Zipkin batch: Bad times ahead\n", statusCode: http.StatusInternalServerError},


nit: this is more readable

{ payload: spanJSON, expected: "Cannot submit Zipkin batch: Bad times ahead\n", statusCode: http.StatusInternalServerError, },

yurishkuro · 2017-08-17T04:53:22Z

cmd/collector/app/zipkin/json.go

+	Annotations       []annotation       `json:"annotations"`
+	BinaryAnnotations []binaryAnnotation `json:"binaryAnnotations"`
+}
+type zipkinSpans []zipkinSpan


only used once, better to inline: var spans []zipkinSpan

It was like that before. @black-adder wanted to move it here. https://github.com/uber/jaeger/pull/334#discussion_r133217513

I think it's fine to define it here as long as it's used in multiple places (which looks like it can). Every reference to []zipkinSpan can be replaced with zipkinSpans. I count at least 3 instances where this happens.

I prefer to use []zipkinSpan directly, less obfuscation. The type alias is useful when you add extra behavior to it, like we do with []model.Tag (sorting, etc.)

yurishkuro · 2017-08-17T04:56:46Z

cmd/collector/app/zipkin/json.go

+	}, nil
+}
+
+func parseIpv4(str string) (int32, error) {


ParseIP returns IP, which is an alias to []byte. If nil return 0 else pack bytes into long

yurishkuro · 2017-08-17T04:58:54Z

cmd/collector/app/zipkin/json.go

+	}
+
+	var result uint64
+	for i := start; i < len; i++ {


why not use model functions like model/TraceIDFromString?

https://github.com/uber/jaeger/blob/master/model/span.go#L146

It automatically takes care of high/low.

pavolloffay · 2017-08-22T08:11:50Z

@yurishkuro I am on PTO until 28th. I don't want to block other people.. If there are no serious blockers I can submit fix PR to address latest comments:

IP parsing
reuse functions from model to handle 128 bit trace ids
minor improvements in http handler

Once this is in #342 could be merged (if it's ok).

pavolloffay requested review from black-adder, oibe, vprithvi and yurishkuro as code owners August 15, 2017 04:41

yurishkuro reviewed Aug 15, 2017

View reviewed changes

yurishkuro mentioned this pull request Aug 15, 2017

Update Zipkin.thrift #335

Closed

pavolloffay force-pushed the zipkin-json branch from a152d1d to 990f499 Compare August 15, 2017 05:57

This was referenced Aug 15, 2017

Cannot find service name in Zipkin span #337

Closed

Merged RPC spans not correctly shown, multi value tags not shown jaegertracing/jaeger-ui#55

Closed

black-adder reviewed Aug 15, 2017

View reviewed changes

pavolloffay force-pushed the zipkin-json branch from 6a83968 to 2ada3b2 Compare August 16, 2017 07:26

pavolloffay added 4 commits August 16, 2017 13:19

Zipkin JSON encoding

8adc8db

fix trivial review comments

31748f3

fix 128bit ids and ipv6 in endpoint

e6da153

fix fmt

cce519b

pavolloffay force-pushed the zipkin-json branch from 23e2b4d to cce519b Compare August 16, 2017 11:19

use fixtures in tests

2603165

test traceids

64499c2

fixtures in span_handler tests

2b406ae

black-adder reviewed Aug 16, 2017

View reviewed changes

fix review comments, define explicit prop names in structs

3211e3d

yurishkuro reviewed Aug 17, 2017

View reviewed changes

yurishkuro mentioned this pull request Aug 22, 2017

Zipkin json encoding via HTTP #348

Merged

pavolloffay closed this Aug 28, 2017

yurishkuro added this to the Drop-in Replacement for Zipkin backend milestone Sep 1, 2017

		@@ -0,0 +1,234 @@
		// Copyright (c) 2017 Uber Technologies, Inc.

Zipkin json encoding via HTTP #334

Zipkin json encoding via HTTP #334

Conversation

pavolloffay commented Aug 15, 2017 • edited

coveralls commented Aug 15, 2017

pavolloffay commented Aug 15, 2017

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Aug 15, 2017 • edited

coveralls commented Aug 15, 2017

coveralls commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

black-adder commented Aug 15, 2017

yurishkuro commented Aug 15, 2017

coveralls commented Aug 16, 2017

coveralls commented Aug 16, 2017

coveralls commented Aug 16, 2017

coveralls commented Aug 16, 2017

coveralls commented Aug 16, 2017

coveralls commented Aug 16, 2017

coveralls commented Aug 16, 2017

pavolloffay commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay Aug 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Aug 22, 2017

pavolloffay commented Aug 15, 2017 •

edited

pavolloffay Aug 15, 2017 •

edited

yurishkuro commented Aug 15, 2017 •

edited

pavolloffay Aug 19, 2017 •

edited