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

This closes #256 by better handling baggage null values #308

Merged
merged 3 commits into from
Feb 1, 2018

Conversation

bmozaffa
Copy link
Contributor

This closes #256 by ignoring attempts to set a new baggage value of null, but also removing existing baggage when their value has been updated as null

Signed-off-by: Babak Mozaffari bmozaffa@redhat.com

…ge value of null, but also removing existing baggage when their value has been updated as null

Signed-off-by: Babak Mozaffari <bmozaffa@redhat.com>
@codecov
Copy link

codecov bot commented Dec 11, 2017

Codecov Report

Merging #308 into master will decrease coverage by 0.07%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #308      +/-   ##
============================================
- Coverage     84.08%   84.01%   -0.08%     
- Complexity      568      570       +2     
============================================
  Files            91       91              
  Lines          2225     2227       +2     
  Branches        258      259       +1     
============================================
  Hits           1871     1871              
- Misses          255      256       +1     
- Partials         99      100       +1
Impacted Files Coverage Δ Complexity Δ
...aeger-core/src/main/java/com/uber/jaeger/Span.java 82.07% <0%> (ø) 39 <2> (+1) ⬆️
...in/java/com/uber/jaeger/baggage/BaggageSetter.java 100% <100%> (ø) 10 <0> (+1) ⬆️
...ore/src/main/java/com/uber/jaeger/SpanContext.java 82.6% <100%> (+0.79%) 25 <0> (+1) ⬆️
...jaeger/reporters/protocols/ThriftUdpTransport.java 81.35% <0%> (-5.09%) 14% <0%> (-2%)
.../uber/jaeger/samplers/RemoteControlledSampler.java 94.36% <0%> (+1.4%) 20% <0%> (+1%) ⬆️

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 8dabf7a...b343f32. 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.

PS - you don't need to keep re-creating PRs with new changes, just push to the same branch

final String value = "value";
SpanContext ctx = setter.setBaggage(span, KEY, value);
Span child = (Span) tracer.buildSpan("some-operation").asChildOf(ctx).startManual();
ctx = setter.setBaggage(child, KEY, null);
Copy link
Member

Choose a reason for hiding this comment

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

I would rather test this via span.setBaggage API, since it has additional null checks.

…checks

Signed-off-by: Babak Mozaffari <bmozaffa@redhat.com>
@ghost ghost assigned yurishkuro Feb 1, 2018
@ghost ghost added the review label Feb 1, 2018
@yurishkuro yurishkuro merged commit c0409b9 into jaegertracing:master Feb 1, 2018
@ghost ghost removed the review label Feb 1, 2018
@yurishkuro
Copy link
Member

thank you!

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

Successfully merging this pull request may close these issues.

Null baggage item causes NPE
2 participants