Skip to content
This repository has been archived by the owner on Jul 11, 2022. It is now read-only.

Add Zipkin API V2 Reporter #197

Closed
wants to merge 2 commits into from

Conversation

rmfitzpatrick
Copy link
Contributor

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 rmfitzpatrick@signalfx.com

@rmfitzpatrick
Copy link
Contributor Author

rmfitzpatrick commented Jul 6, 2018

Fixing the issues found in integration.

edit: Issues resolved.

@codecov
Copy link

codecov bot commented Jul 6, 2018

Codecov Report

Merging #197 into master will increase coverage by 0.31%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   94.75%   95.06%   +0.31%     
==========================================
  Files          25       26       +1     
  Lines        1773     1926     +153     
  Branches      224      256      +32     
==========================================
+ Hits         1680     1831     +151     
+ Misses         60       54       -6     
- Partials       33       41       +8
Impacted Files Coverage Δ
jaeger_client/local_agent_net.py 95.65% <100%> (+0.3%) ⬆️
crossdock/server/endtoend.py 100% <100%> (ø) ⬆️
jaeger_client/config.py 93.15% <90%> (+2.46%) ⬆️
jaeger_client/zipkin_v2.py 91% <91%> (ø)
jaeger_client/reporter.py 95.31% <94.87%> (+3.43%) ⬆️

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 00d3d4a...ce6fb0b. Read the comment docs.

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 <rmfitzpatrick@signalfx.com>
@rmfitzpatrick rmfitzpatrick force-pushed the zipkin_v2 branch 4 times, most recently from 471709b to b3b5226 Compare July 9, 2018 15:41
Signed-off-by: Ryan Fitzpatrick <rmfitzpatrick@signalfx.com>
@rmfitzpatrick
Copy link
Contributor Author

I've increased test coverage for all proposed changes, including some potentially overreaching reporter tests. Apologies for the churn and any feedback would be greatly appreciated.

@rmfitzpatrick
Copy link
Contributor Author

@black-adder and @yurishkuro, in light of jaegertracing/jaeger#925 understandably preventing these changes from being accepted, if a short term requirement is a thrift over http reporter in the client do you think it's reasonable for me to:

  1. Attempt to add the append and flush methods to Preliminary refactor for supporting spans over HTTP #186 for that to land.
  2. Propose an HTTPSender, while using the LocalAgentReader distinction proposed in this PR?

@black-adder
Copy link
Contributor

@rmfitzpatrick I think that'd be great.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants