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
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6dd8b67
This closes #256 by ignoring attempts to set a new baggage value of n…
bmozaffa Oct 7, 2017
83fc8fb
Using span.setBaggage API in unit test, since it has additional null …
bmozaffa Dec 11, 2017
b343f32
Merge branch 'master' into handle_baggage_null_value
yurishkuro Feb 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion jaeger-core/src/main/java/com/uber/jaeger/Span.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ public List<LogData> getLogs() {

@Override
public Span setBaggageItem(String key, String value) {
if (key == null || value == null) {
if (key == null || (value == null && context.getBaggageItem(key) == null)) {
//Ignore attempts to add new baggage items with null values, they're not accessible anyway
return this;
}
synchronized (this) {
Expand Down
6 changes: 5 additions & 1 deletion jaeger-core/src/main/java/com/uber/jaeger/SpanContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,11 @@ public static SpanContext contextFromString(String value)

public SpanContext withBaggageItem(String key, String val) {
Map<String, String> newBaggage = new HashMap<String, String>(this.baggage);
newBaggage.put(key, val);
if (val == null) {
newBaggage.remove(key);
} else {
newBaggage.put(key, val);
}
return new SpanContext(traceId, spanId, parentId, flags, newBaggage, debugId);
}

Expand Down
2 changes: 1 addition & 1 deletion jaeger-core/src/main/java/com/uber/jaeger/baggage/BaggageSetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public SpanContext setBaggage(Span span, String key, String value) {
logFields(span, key, value, prevItem, truncated, restriction.isKeyAllowed());
return span.context();
}
if (value.length() > restriction.getMaxValueLength()) {
if (value != null && value.length() > restriction.getMaxValueLength()) {
truncated = true;
value = value.substring(0, restriction.getMaxValueLength());
metrics.baggageTruncate.inc(1);
Expand Down
25 changes: 25 additions & 0 deletions jaeger-core/src/test/java/com/uber/jaeger/baggage/BaggageSetterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,31 @@ public void testUnsampledSpan() {
assertNull(span.getLogs());
}

@Test
public void testBaggageNullValueTolerated() {
when(mgr.getRestriction(SERVICE, KEY)).thenReturn(Restriction.of(true, 5));
final String value = null;
SpanContext ctx = setter.setBaggage(span, KEY, value);

assertBaggageLogs(span, KEY, null, false, false, false);
assertNull(ctx.getBaggageItem(KEY));
}

@Test
public void testBaggageNullRemoveValue() {
when(mgr.getRestriction(SERVICE, KEY)).thenReturn(Restriction.of(true, 5));
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.


assertBaggageLogs(child, KEY, null, false, true, false);
assertNull(ctx.getBaggageItem(KEY));

assertEquals(
2L, metricsReporter.counters.get("jaeger.baggage-update.result=ok").longValue());
}

private void assertBaggageLogs(
Span span,
String key,
Expand Down