Support for Zipkin Protobuf spans over HTTP #1695

jan25 · 2019-07-26T21:10:43Z

Which problem is this PR solving?

Adds support in collector for Zipkin Protobuf spans sent to /api/v2/spans
Fixes Add support for Zipkin Protobuf spans over HTTP #1586

Short description of the changes

We had JSON support for Zipkin spans sent over HTTP to v2 endpoint, this PR is a mirrored implementation based on jsonv2 implementation
Related PRs that should be merged to complete adding zipkin proto support: Add Protobuf span reporter xdock-zipkin-brave#13 and Add zipkin.proto jaeger-idl#56
~~Temporarily marking this PR as WIP. Please review/suggest on the implementation~~

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

yurishkuro

I am concerned by importing zipkin-go that brings with it dependencies on protobuf with potentially conflicting versions. Why can't we just use the zipkin proto file and generate the model ourselves? As far as I can tell there is no dependency on any other logic from zipkin-go.

cmd/collector/app/zipkin/protov2.go

jan25 · 2019-07-27T04:51:38Z

@yurishkuro Thanks!
Logic-wise, no difference except the type definitions changing a little. I thought zipkin-go is officially supported/made for different collector like Jaeger.

You have a fair point about us having to control which proto version we want to support. I can update this PR to use proto file directly and generate code, and jaeger-idl looks like ideal place to put this proto file.

yurishkuro · 2019-07-29T23:17:47Z

Yes, the proto file can go to jaeger-idl

pavolloffay · 2019-07-30T08:15:42Z

Manual test the v2 endpoint with zipkin proto spans

Have a look at xdock tests, there are already tests for zipkin thrift and v2 json. We could leverage the same approach and add proto. It will require adding support for proto reporter in https://github.com/jaegertracing/xdock-zipkin-brave

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

codecov · 2019-08-10T19:50:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@581f9be). Click here to learn what that means.
The diff coverage is 97.61%.

@@           Coverage Diff            @@
##             master   #1695   +/-   ##
========================================
  Coverage          ?   98.3%           
========================================
  Files             ?     194           
  Lines             ?    9521           
  Branches          ?       0           
========================================
  Hits              ?    9360           
  Misses            ?     126           
  Partials          ?      35

Impacted Files	Coverage Δ
model/ids.go	`100% <ø> (ø)`
cmd/collector/app/zipkin/http_handler.go	`100% <100%> (ø)`
cmd/collector/app/zipkin/protov2.go	`97.16% <97.16%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 581f9be...e5168bb. Read the comment docs.

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

jan25 · 2019-08-10T21:00:56Z

I missed make install-tools, thanks for the fix!

pavolloffay

Could you please also enable the xdock tests here?

cmd/collector/app/zipkin/protov2.go

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

jan25 · 2019-08-13T07:14:17Z

@pavolloffay thanks! Added them now. I think we should merge jaegertracing/xdock-zipkin-brave#13 to go forward with this main PR.

pavolloffay · 2019-08-13T07:29:56Z

@jan25 I have added there one comment, otherwise it looks good to me.

yurishkuro

looks like crossdock failed on the new test (as well as on e2e test, but that one could be a bit flaky)

otherwise lgtm

yurishkuro · 2019-08-15T16:27:28Z

cmd/collector/app/zipkin/protov2.go

+	return tSpan, nil
+}
+
+func traceIDFromBytes(tid []byte) (model.TraceID, error) {


Perhaps incorporate this into func (t *TraceID) Unmarshal(data []byte) error instead. In which case the *BytesLen constants can remain private

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

pavolloffay · 2019-08-16T09:48:36Z

thanks @jan25

jan25 · 2019-08-16T11:39:23Z

Thanks for all the help! @yurishkuro I was working on your comment in the review :) but this is already merged, will open different PR when i have that refactored.

pavolloffay · 2019-08-16T11:57:04Z

I have considered it as fixed due to 👍 mark on the comment. thanks @jan25

pavolloffay · 2019-08-16T11:58:53Z

@jan25 are you using Jaeger in booking.com? If yes could you please add an entry to https://github.com/jaegertracing/jaeger/blob/master/ADOPTERS.md or at least comment in #207

jan25 · 2019-08-16T19:25:08Z

@pavolloffay Not yet, But noted the links from your previous comment to update them when booking.com does :) thanks!

pavolloffay · 2019-08-19T07:47:21Z

thanks @jan25 !

jan25 added 7 commits July 26, 2019 22:58

WIP handle if block to handle zipkin proto v2

9c9f4b2

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

update deps

387049f

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

WIP add proto to thrift convertor

21c149b

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

fix converter

3697be3

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

wrap protoV2 to Thrift span convderter

993791c

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

refactor http-handler for json and proto content type

ab51207

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

fix spanV2 tests

df948d1

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

yurishkuro reviewed Jul 26, 2019

View reviewed changes

cmd/collector/app/zipkin/protov2.go Outdated Show resolved Hide resolved

jan25 added 5 commits July 30, 2019 19:51

generate zipkin proto models

b42bd79

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

convert proto spans to thrift

f949e74

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

remove zipkin-go dep

15f6434

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

add fixture test

5bcfb2f

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

refactor: detach fns from api handler

a04e8ba

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

This was referenced Aug 4, 2019

Add Protobuf span reporter jaegertracing/xdock-zipkin-brave#13

Merged

Add zipkin.proto jaegertracing/jaeger-idl#56

Merged

jan25 added 5 commits August 7, 2019 09:26

improve tests WIP

9252d9c

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

fix main fixture test

367fe5c

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

add ids consts

11901e4

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

fix trace id high

65e9fe7

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

fix static checks

23e5575

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

jan25 changed the title ~~[WIP] Support for Zipkin Protobuf spans over HTTP~~ Support for Zipkin Protobuf spans over HTTP Aug 7, 2019

jan25 marked this pull request as ready for review August 7, 2019 21:03

jan25 requested review from black-adder, jpkrohling, objectiser, pavolloffay and tiffon as code owners August 7, 2019 21:03

bump idl with zipkin proto

68b2f34

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

jan25 added 3 commits August 10, 2019 21:59

rebase idl fork to get zipkin.proto

9662b37

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

tests for proto span kind

df1d225

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

rename zmodel->zipkinProto

8bf5cf8

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

Merge branch 'master' into add-http-proto

8f5a824

pavolloffay reviewed Aug 12, 2019

View reviewed changes

cmd/collector/app/zipkin/protov2.go Outdated Show resolved Hide resolved

jan25 added 3 commits August 13, 2019 00:17

add xdock proto

fdded94

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

minor: better error strs

614a90c

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

typo

380eeff

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

pavolloffay mentioned this pull request Aug 14, 2019

Release 1.14.0 #1732

Closed

1 task

yurishkuro reviewed Aug 15, 2019

View reviewed changes

jan25 added 2 commits August 15, 2019 20:48

Merge branch 'master' into add-http-proto

de23c73

rename zipkin-brave-proto-v2 -> zipkin-brave-proto

e5168bb

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>

pavolloffay approved these changes Aug 16, 2019

View reviewed changes

pavolloffay merged commit a016891 into jaegertracing:master Aug 16, 2019

jan25 deleted the add-http-proto branch August 19, 2019 15:59

jan25 mentioned this pull request Aug 20, 2019

Refactor zipkin/protov2.go #1747

Merged

jan25 mentioned this pull request Aug 28, 2019

Update Zipkin related docs in v1.14 jaegertracing/documentation#292

Closed

jan25 mentioned this pull request Sep 7, 2019

Update docs for Jaeger backend openzipkin/openzipkin.github.io#145

Merged

jan25 mentioned this pull request Oct 12, 2019

Update docs for Zipkin Proto support jaegertracing/documentation#313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Zipkin Protobuf spans over HTTP #1695

Support for Zipkin Protobuf spans over HTTP #1695

jan25 commented Jul 26, 2019 •

edited

Loading

yurishkuro left a comment

jan25 commented Jul 27, 2019

yurishkuro commented Jul 29, 2019

pavolloffay commented Jul 30, 2019

codecov bot commented Aug 10, 2019 •

edited

Loading

jan25 commented Aug 10, 2019

pavolloffay left a comment

jan25 commented Aug 13, 2019

pavolloffay commented Aug 13, 2019

yurishkuro left a comment

yurishkuro Aug 15, 2019

pavolloffay commented Aug 16, 2019

jan25 commented Aug 16, 2019

pavolloffay commented Aug 16, 2019

pavolloffay commented Aug 16, 2019

jan25 commented Aug 16, 2019

pavolloffay commented Aug 19, 2019

Support for Zipkin Protobuf spans over HTTP #1695

Support for Zipkin Protobuf spans over HTTP #1695

Conversation

jan25 commented Jul 26, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

yurishkuro left a comment

Choose a reason for hiding this comment

jan25 commented Jul 27, 2019

yurishkuro commented Jul 29, 2019

pavolloffay commented Jul 30, 2019

codecov bot commented Aug 10, 2019 • edited Loading

Codecov Report

jan25 commented Aug 10, 2019

pavolloffay left a comment

Choose a reason for hiding this comment

jan25 commented Aug 13, 2019

pavolloffay commented Aug 13, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Aug 15, 2019

Choose a reason for hiding this comment

pavolloffay commented Aug 16, 2019

jan25 commented Aug 16, 2019

pavolloffay commented Aug 16, 2019

pavolloffay commented Aug 16, 2019

jan25 commented Aug 16, 2019

pavolloffay commented Aug 19, 2019

jan25 commented Jul 26, 2019 •

edited

Loading

codecov bot commented Aug 10, 2019 •

edited

Loading