Collect Zipkin v2 json by pavolloffay · Pull Request #518 · jaegertracing/jaeger

pavolloffay · 2017-11-08T14:27:25Z

Fixes #450

How does it work:

zipkin swagger from idl is compiled to go code (it uses https://github.com/go-swagger/go-swagger)
custom http handler which uses generated files to unmarshall and validate spans
convert swagger model classes to zipkin thrift

Zipkin related links:
messaging annos: openzipkin/zipkin-api#29
span v2: openzipkin/zipkin#1499

coveralls · 2017-11-08T17:18:34Z

Coverage decreased (-1.3%) to 98.656% when pulling 4b12db5 on pavolloffay:zipkin-v2 into 2b73fe9 on jaegertracing:master.

yurishkuro · 2017-11-09T04:28:02Z

.gitmodules

 	url = https://github.com/uber/jaeger-ui
+[submodule "zipkin-api"]
+	path = zipkin-api
+	url = https://github.com/openzipkin/zipkin-api


What exactly are we getting from zipkin? I would rather do it without submodule dependency

If it's just one file we should simply copy it, similar to how we copy the thrift IDL file.

It's only one file https://github.com/openzipkin/zipkin-api/blob/master/zipkin2-api.yaml The repo only contains API definitions: thrift and swagger.

I wanted to propose to use that also for zipkin.thrift. Or we can just simply copy swagger to our idl. I don't have a strong preference. Zipkin submodule is easier to maintain. For example our zipkin thrift definition file slightly differs from upstream.

jaegertracing/jaeger-idl#39

coveralls · 2017-11-10T13:31:24Z

Coverage decreased (-2.3%) to 97.716% when pulling cd795b5 on pavolloffay:zipkin-v2 into a2ed9b8 on jaegertracing:master.

pavolloffay · 2017-11-10T14:05:57Z

xdock tests with brave using JSON V2 are passing. It can be reviewed. In meanwhile I will improve coverage and wait for swagger a thrift IDL changes.

coveralls · 2017-11-13T12:46:49Z

Coverage decreased (-2.2%) to 97.716% when pulling 102d6cf on pavolloffay:zipkin-v2 into 32e2cd3 on jaegertracing:master.

pavolloffay · 2017-11-13T18:45:09Z

it's ready for review

coveralls · 2017-11-13T19:01:00Z

Coverage remained the same at 100.0% when pulling 43b8461 on pavolloffay:zipkin-v2 into e0a74f2 on jaegertracing:master.

black-adder · 2017-11-14T14:34:45Z

cmd/collector/app/zipkin/http_handler.go

+	if err != nil {
+		return nil, err
+	}
+	defer gz.Close()


won't this close gz at the end of this function? looking at the tests, it looks like it works fine? hmmm

black-adder · 2017-11-14T15:34:17Z

cmd/collector/app/zipkin/http_handler.go

 func NewAPIHandler(
 	zipkinSpansHandler app.ZipkinSpansHandler,
-) *APIHandler {
+) (*APIHandler, error) {


this function doesn't seem to return an error, is the API change necessary?

black-adder · 2017-11-14T15:49:45Z

cmd/collector/app/zipkin/jsonv2.go

+	"github.com/jaegertracing/jaeger/thrift-gen/zipkincore"
+)
+
+func spansV2ToThrift(spans *models.ListOfSpans) ([]*zipkincore.Span, error) {


is the pointer to the slice required? It doesn't look like you're doing anything to the slice reference in this function. Without the pointer, this function should be more readable

black-adder · 2017-11-14T15:53:59Z

cmd/collector/app/zipkin/jsonv2.go

+	return tSpans, nil
+}
+
+func spanV2ToThrift(s models.Span) (*zipkincore.Span, error) {


when this function is invoked on L26, you already have a pointer to the span. Why not pass the pointer rather than making a copy of the span and passing that by value?

black-adder · 2017-11-14T15:57:33Z

cmd/collector/app/zipkin/jsonv2.go

+}
+
+func annoV2ToThrift(a *models.Annotation, e *zipkincore.Endpoint) *zipkincore.Annotation {
+	ta := &zipkincore.Annotation{


looks like you can inline return

black-adder · 2017-11-14T15:58:04Z

cmd/collector/app/zipkin/jsonv2.go

+}
+
+func tagsToThrift(tags models.Tags, localE *zipkincore.Endpoint) []*zipkincore.BinaryAnnotation {
+	var bAnnos []*zipkincore.BinaryAnnotation


preallocate bAnnos to len(tags)

black-adder · 2017-11-14T15:59:02Z

crossdock/docker-compose.yml

        environment:
            - ENCODING=JSON

+    zipkin-brave-json-v2:


+1 love how easy it is to integration test new endpoints

coveralls · 2017-11-15T10:35:59Z

Coverage remained the same at 100.0% when pulling ca70179 on pavolloffay:zipkin-v2 into e0a74f2 on jaegertracing:master.

black-adder · 2017-11-15T14:32:34Z

cmd/collector/app/zipkin/jsonv2.go

+)
+
+func spansV2ToThrift(spans models.ListOfSpans) ([]*zipkincore.Span, error) {
+	var tSpans []*zipkincore.Span


this can be preallocated too

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

coveralls · 2017-11-15T15:13:31Z

Coverage remained the same at 100.0% when pulling 78db314 on pavolloffay:zipkin-v2 into 5db9cde on jaegertracing:master.

coveralls · 2017-11-15T15:23:01Z

Coverage remained the same at 100.0% when pulling 78db314 on pavolloffay:zipkin-v2 into 5db9cde on jaegertracing:master.

pavolloffay · 2017-11-16T08:25:49Z

@yurishkuro I will merge on green, I can apply additional changes in a separate PR.

coveralls · 2017-11-16T08:33:44Z

Coverage remained the same at 100.0% when pulling 3a703db on pavolloffay:zipkin-v2 into 6897ca2 on jaegertracing:master.

pavolloffay requested review from black-adder, vprithvi and yurishkuro as code owners November 8, 2017 14:27

ghost assigned pavolloffay Nov 8, 2017

ghost added the review label Nov 8, 2017

pavolloffay force-pushed the zipkin-v2 branch from 008bc7d to dcdba0e Compare November 8, 2017 15:38

yurishkuro reviewed Nov 9, 2017

View reviewed changes

pavolloffay force-pushed the zipkin-v2 branch from 91e99be to ccb2cad Compare November 10, 2017 11:51

pavolloffay force-pushed the zipkin-v2 branch from cd795b5 to 102d6cf Compare November 13, 2017 12:37

pavolloffay force-pushed the zipkin-v2 branch from 102d6cf to 8cd5aed Compare November 13, 2017 17:27

pavolloffay changed the title ~~WIP: Collect Zipkin v2 json~~ Collect Zipkin v2 json Nov 13, 2017

pavolloffay mentioned this pull request Nov 13, 2017

Change package from uber to jaegertracing #528

Merged

black-adder reviewed Nov 14, 2017

View reviewed changes

black-adder approved these changes Nov 15, 2017

View reviewed changes

pavolloffay added 7 commits November 15, 2017 16:03

Collect Zipkin v2 json

015a3c7

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

fix make

8cf9512

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

remove submodule

7d107b1

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Trasform messaging spans

19529fb

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

http handler tests

ccc27ba

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix review comments

080a7dd

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Preallocate spans slice

78db314

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the zipkin-v2 branch from 90d3156 to 78db314 Compare November 15, 2017 15:03

Merge branch 'master' into zipkin-v2

3a703db

pavolloffay merged commit 53955e3 into jaegertracing:master Nov 16, 2017

ghost removed the review label Nov 16, 2017

yurishkuro mentioned this pull request Dec 14, 2017

localEndpoint is not parsed for extraction of service name #585

Closed

yurishkuro mentioned this pull request May 5, 2018

Move Zipkin backports to another repository? #807

Open

Conversation

pavolloffay commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 10, 2017

Uh oh!

pavolloffay commented Nov 10, 2017

Uh oh!

coveralls commented Nov 13, 2017

Uh oh!

pavolloffay commented Nov 13, 2017

Uh oh!

coveralls commented Nov 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 15, 2017

Uh oh!

coveralls commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavolloffay commented Nov 16, 2017

Uh oh!

coveralls commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pavolloffay commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 15, 2017 •

edited

Loading