Add Zipkin v2 API reporter #310

keitwb · 2018-05-14T18:15:36Z

Which problem is this PR solving?

Lack of Zipkin v2 support

Short description of the changes

This reporter wraps an instance of a Zipkin reporter, which closely
matches the interface for Jaeger reporters, so the adaptation is very
simple.

Most of the complexity lies in converting from a Jaeger span to a Zipkin
v2 span.

The existing Zipkin v1 Thrift code is left untouched.

codecov · 2018-05-14T18:25:56Z

Codecov Report

Merging #310 into master will increase coverage by 0.25%.
The diff coverage is 95%.

@@            Coverage Diff            @@
##           master    #310      +/-   ##
=========================================
+ Coverage   87.15%   87.4%   +0.25%     
=========================================
  Files          54      57       +3     
  Lines        2965    3065     +100     
=========================================
+ Hits         2584    2679      +95     
- Misses        269     273       +4     
- Partials      112     113       +1

Impacted Files	Coverage Δ
reporter.go	`100% <ø> (ø)`	⬆️
zipkin/reporter.go	`100% <100%> (ø)`
internal/utils.go	`100% <100%> (ø)`
zipkin_v2_span.go	`94.56% <94.56%> (ø)`

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 f7e0d47...79fc25a. Read the comment docs.

vprithvi · 2018-05-14T22:17:33Z

Thanks for the PR! I'll take a look in a bit

black-adder · 2018-05-16T16:51:23Z

I'm curious, are you currently using jaeger client with a zipkin backend so that you can use the opentracing api?

keitwb · 2018-05-16T19:43:45Z

Yes, the current relationship between Zipkin and Opentracing is pretty tenuous (e.g. opentracing/opentracing.io#258) and we'd rather use a tracer that is fully committed to OpenTracing for the long run, even though Zipkin does have an opentracing wrapper available.

black-adder · 2018-05-16T19:51:13Z

Understood, I'm gonna have to play around this a bit because I'd rather not have the github.com/openzipkin/zipkin-go dependency pulled in for everyone using this client. If we could somehow vendor the github.com/openzipkin/zipkin-go dependency so that it's only pulled in if you configure the tracer to use the zipkin reporter then we'll be golden.

yurishkuro · 2018-05-16T19:51:19Z

@keitwb why not use Jaeger as the backend too? Operation reasons or features?

yurishkuro · 2018-05-16T19:56:43Z

zipkin_v2_span.go

+)
+
+// BuildZipkinV2Span converts a Jaeger span to a Zipkin v2 span
+func BuildZipkinV2Span(span *Span) *zipkinModel.SpanModel {


is there a way to stash these files under zipkin/ package? We don't want to increase the footprint of the client for users who only use it with non-zipkin parts

The only way I see to be able to move it to the zipkin package is to add several accessor methods on the jaeger Span struct that we need to do the conversion (e.g. logs, duration, tags are all unexported fields of Span).

Doesn't seem too hard to pull off, but I'm not sure if it is ok to augment the public interface of the Span struct so much.

@black-adder I think this would also allow the zipkin-go lib to be only pulled in if projects were using code from the zipkin subpackage (at least this is my understanding of how glide/dep work).

yurishkuro · 2018-05-16T19:57:47Z

utils/utils.go

@@ -75,6 +75,13 @@ func PackIPAsUint32(ip net.IP) uint32 {
 	return 0
 }

+// UnpackUint32AsIP does the reverse of PackIPAsUint32
+func UnpackUint32AsIP(ip uint32) net.IP {


please move new functions to internal/utils.go. we don't want to be adding more public methods that are not part of the client API.

keitwb · 2018-05-17T15:07:26Z

@yurishkuro As far as why we don't use Jaeger on the backend, it's more just a matter that we got established with Zipkin a while back and it suits our purposes well enough so far. I wasn't part of the team that originally chose Zipkin over the other backends so I can't speak to why they chose it in the first place. We really like the OpenTracing initiative though and are sorry to see that Zipkin seems to be moving away from it.

golangci · 2018-05-17T15:09:59Z

zipkin_v2_span.go

+				kind = zipkinModel.Consumer
+			}
+		default:
+			tags[string(tag.key)] = stringify(tag.value)


unnecessary conversion

golangci · 2018-05-17T15:10:34Z

zipkin_v2_span.go

+				kind = zipkinModel.Consumer
+			}
+		default:
+			tags[string(tag.key)] = stringify(tag.value)


unnecessary conversion

This reporter wraps an instance of a Zipkin reporter, which closely matches the interface for Jaeger reporters, so the adaptation is very simple. Most of the complexity lies in converting from a Jaeger span to a Zipkin v2 span. The existing Zipkin v1 Thrift code is left untouched. Signed-off-by: Ben Keith <[email protected]>

Signed-off-by: Ben Keith <[email protected]>

Expands span reporter selection by breaking out Reporter into base QueueReporter class and using reporting method-specific ThriftReporter and ZipkinV2Reporter. Also breaks LocalAgentSender into two classes including LocalAgentReader for ThriftReporter only usage of TBufferedTransport. These changes allow users to continue reporting spans oob to the local jaeger-agent but with the option of reporting to Zipkin backend. In the spirit of jaegertracing/jaeger-client-java#399 and jaegertracing/jaeger-client-go#310 Signed-off-by: Ryan Fitzpatrick <[email protected]>

keitwb · 2018-07-24T20:22:18Z

We can solve our use-case by using the Jaeger HTTP transport added in #161. I'll leave the branch on our repo for a bit in case anybody else finds it useful but am closing this PR for now.

keitwb force-pushed the zipkin2-reporter branch 3 times, most recently from 33f6d59 to 7df0db7 Compare May 14, 2018 21:00

yurishkuro reviewed May 16, 2018

View reviewed changes

golangci reviewed May 17, 2018

View reviewed changes

zipkin_v2_span.go Outdated

kind = zipkinModel.Consumer

}

default:

tags[string(tag.key)] = stringify(tag.value)

Copy link

golangci May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary conversion

keitwb force-pushed the zipkin2-reporter branch from f1e5ce0 to e700adf Compare May 17, 2018 15:10

golangci reviewed May 17, 2018

View reviewed changes

zipkin_v2_span.go Outdated

kind = zipkinModel.Consumer

}

default:

tags[string(tag.key)] = stringify(tag.value)

Copy link

golangci May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary conversion

pavolloffay mentioned this pull request May 18, 2018

Implement Zipkin 2 JSON Sender jaegertracing/jaeger-client-java#399

Merged

Ben Keith added 3 commits June 22, 2018 13:22

Moving UnpackUint32AsIP to internal package

305eab0

Signed-off-by: Ben Keith <[email protected]>

Removing unnecessary string conversion

5d6fde1

Signed-off-by: Ben Keith <[email protected]>

keitwb force-pushed the zipkin2-reporter branch from efbc068 to 189a3d6 Compare June 22, 2018 17:29

Updating truncateString in zipkin v2 converter

79fc25a

Signed-off-by: Ben Keith <[email protected]>

keitwb force-pushed the zipkin2-reporter branch from 189a3d6 to 79fc25a Compare June 22, 2018 17:30

rmfitzpatrick mentioned this pull request Jul 6, 2018

Add Zipkin API V2 Reporter jaegertracing/jaeger-client-python#197

Closed

black-adder mentioned this pull request Jul 10, 2018

Move zipkin reporting logic to jaeger agent/collector jaegertracing/jaeger#925

Closed

keitwb closed this Jul 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Zipkin v2 API reporter #310

Add Zipkin v2 API reporter #310

keitwb commented May 14, 2018 •

edited

Loading

codecov bot commented May 14, 2018 •

edited

Loading

vprithvi commented May 14, 2018

black-adder commented May 16, 2018

keitwb commented May 16, 2018

black-adder commented May 16, 2018

yurishkuro commented May 16, 2018

yurishkuro May 16, 2018

keitwb May 17, 2018 •

edited

Loading

keitwb May 17, 2018

yurishkuro May 16, 2018

keitwb commented May 17, 2018

golangci May 17, 2018

golangci May 17, 2018

keitwb commented Jul 24, 2018

Add Zipkin v2 API reporter #310

Add Zipkin v2 API reporter #310

Conversation

keitwb commented May 14, 2018 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented May 14, 2018 • edited Loading

Codecov Report

vprithvi commented May 14, 2018

black-adder commented May 16, 2018

keitwb commented May 16, 2018

black-adder commented May 16, 2018

yurishkuro commented May 16, 2018

yurishkuro May 16, 2018

Choose a reason for hiding this comment

keitwb May 17, 2018 • edited Loading

Choose a reason for hiding this comment

keitwb May 17, 2018

Choose a reason for hiding this comment

yurishkuro May 16, 2018

Choose a reason for hiding this comment

keitwb commented May 17, 2018

golangci May 17, 2018

Choose a reason for hiding this comment

golangci May 17, 2018

Choose a reason for hiding this comment

keitwb commented Jul 24, 2018

keitwb commented May 14, 2018 •

edited

Loading

codecov bot commented May 14, 2018 •

edited

Loading

keitwb May 17, 2018 •

edited

Loading