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

Support span log fields in zipkin sender #226

Merged
merged 2 commits into from
Aug 2, 2017

Conversation

pvlugter
Copy link
Contributor

@pvlugter pvlugter commented Aug 2, 2017

We're using the Jaeger client to support reporting to both Jaeger and Zipkin. The Zipkin sender currently fails on spans with logs using fields, when the Annotation value (from the LogData message) is null. Recent versions of the Jaeger client also log structured data when baggage is attached, so the Zipkin sender fails on spans with baggage too.

Changes here check whether there is a message or fields, and create a single string message from the structured data fields. An alternative would be to just have the null check and only add annotations for simple message logs.

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2017

CLA assistant check
All committers have signed the CLA.

}
}

return annotations;
}

private static String logFieldsAsMessage(Map<String, ?> logFields) {
Copy link
Member

Choose a reason for hiding this comment

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

In other languages we used JSON encoding for structured data when converting to Zipkin. The Zipkin UI understands those and displays then better too. The openracing library for Zipkin (at least in Go) also does JSON encoding of KV logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even better. Will switch to a JSON encoding for this. Looks like Gson is on the classpath already.

@codecov-io
Copy link

codecov-io commented Aug 2, 2017

Codecov Report

Merging #226 into master will increase coverage by 0.21%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #226      +/-   ##
============================================
+ Coverage      81.2%   81.42%   +0.21%     
- Complexity      490      494       +4     
============================================
  Files            79       79              
  Lines          1889     1895       +6     
  Branches        227      229       +2     
============================================
+ Hits           1534     1543       +9     
+ Misses          259      258       -1     
+ Partials         96       94       -2
Impacted Files Coverage Δ Complexity Δ
...ber/jaeger/senders/zipkin/ThriftSpanConverter.java 84.94% <85.71%> (-0.12%) 26 <0> (+1)
.../uber/jaeger/samplers/RemoteControlledSampler.java 92.95% <0%> (+1.4%) 19% <0%> (+1%) ⬆️
...jaeger/reporters/protocols/ThriftUdpTransport.java 83.05% <0%> (+5.08%) 16% <0%> (+2%) ⬆️

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 e2c6d1a...0e0f3da. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm

@vprithvi please merge if looks ok

@yurishkuro
Copy link
Member

@vprithvi
Copy link
Contributor

vprithvi commented Aug 2, 2017

lgtm

@vprithvi vprithvi merged commit ec6ac78 into jaegertracing:master Aug 2, 2017
@yurishkuro
Copy link
Member

@pvlugter thanks for the fix!

@pvlugter pvlugter deleted the zipkin-log-fields branch August 2, 2017 19:39
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.

None yet

5 participants