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

Add BaggageRestrictionManager #217

Merged
19 changes: 6 additions & 13 deletions jaeger-core/src/main/java/com/uber/jaeger/Span.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

package com.uber.jaeger;

import com.uber.jaeger.baggage.BaggageValidity;
import io.opentracing.tag.Tags;
import java.util.ArrayList;
import java.util.Collections;
Expand Down Expand Up @@ -117,21 +118,13 @@ public List<LogData> getLogs() {
@Override
public Span setBaggageItem(String key, String value) {
synchronized (this) {
// TODO emit a metric whenever baggage is updated
String prevItem = this.getBaggageItem(key);
this.context = this.context.withBaggageItem(key, value);
if (context.isSampled()) {
Map<String, String> fields = new HashMap<String, String>();
fields.put("event", "baggage");
fields.put("key", key);
fields.put("value", value);
if (prevItem != null) {
fields.put("override", "true");
}
return this.log(fields);
if (key == null || value == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't need to be synchronized.

return this;
}
BaggageValidity baggageValidity = this.getTracer().getBaggageRestrictionManager().isBaggageValid(key, value);
Copy link
Member

@yurishkuro yurishkuro Jul 24, 2017

Choose a reason for hiding this comment

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

how about

  • s/BaggageValidity/BaggageRestriction/
  • s/isBaggageValid(key, value)/getBaggageRestriction(key)/
  • s/sanitizeBaggage/setBaggage/
  • move the call to getBaggageRestriction outside of synchronized since it's not dependent on the span

Copy link
Member

Choose a reason for hiding this comment

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

or s/BaggageValidity/BaggageSetter/ (back to "strategy" pattern)

this.context = baggageValidity.sanitizeBaggage(this, key, value);
return this;
}
return this;
}

@Override
Expand Down
20 changes: 17 additions & 3 deletions jaeger-core/src/main/java/com/uber/jaeger/Tracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@

package com.uber.jaeger;

import com.uber.jaeger.Constants;
import com.uber.jaeger.baggage.BaggageRestrictionManager;
import com.uber.jaeger.baggage.DefaultBaggageRestrictionManager;
import com.uber.jaeger.exceptions.UnsupportedFormatException;
import com.uber.jaeger.metrics.Metrics;
import com.uber.jaeger.metrics.NullStatsReporter;
Expand Down Expand Up @@ -74,6 +75,7 @@ public class Tracer implements io.opentracing.Tracer {
private final Map<String, ?> tags;
private final boolean zipkinSharedRpcSpan;
private final ActiveSpanSource activeSpanSource;
private final BaggageRestrictionManager baggageRestrictionManager;

private Tracer(
String serviceName,
Expand All @@ -84,7 +86,8 @@ private Tracer(
Metrics metrics,
Map<String, Object> tags,
boolean zipkinSharedRpcSpan,
ActiveSpanSource activeSpanSource) {
ActiveSpanSource activeSpanSource,
BaggageRestrictionManager baggageRestrictionManager) {
this.serviceName = serviceName;
this.reporter = reporter;
this.sampler = sampler;
Expand All @@ -93,6 +96,7 @@ private Tracer(
this.metrics = metrics;
this.zipkinSharedRpcSpan = zipkinSharedRpcSpan;
this.activeSpanSource = activeSpanSource;
this.baggageRestrictionManager = baggageRestrictionManager;

this.version = loadVersion();

Expand Down Expand Up @@ -140,6 +144,10 @@ Reporter getReporter() {
return reporter;
}

BaggageRestrictionManager getBaggageRestrictionManager() {
return baggageRestrictionManager;
}

void reportSpan(Span span) {
reporter.report(span);
metrics.spansFinished.inc(1);
Expand Down Expand Up @@ -448,6 +456,7 @@ public static final class Builder {
private Map<String, Object> tags = new HashMap<String, Object>();
private boolean zipkinSharedRpcSpan;
private ActiveSpanSource activeSpanSource = new ThreadLocalActiveSpanSource();
private BaggageRestrictionManager baggageRestrictionManager = new DefaultBaggageRestrictionManager();

public Builder(String serviceName, Reporter reporter, Sampler sampler) {
if (serviceName == null || serviceName.trim().length() == 0) {
Expand Down Expand Up @@ -524,9 +533,14 @@ public Builder withTags(Map<String, String> tags) {
return this;
}

public Builder withBaggageRestrictionManager(BaggageRestrictionManager baggageRestrictionManager) {
this.baggageRestrictionManager = baggageRestrictionManager;
return this;
}

public Tracer build() {
return new Tracer(this.serviceName, reporter, sampler, registry, clock, metrics, tags,
zipkinSharedRpcSpan, activeSpanSource);
zipkinSharedRpcSpan, activeSpanSource, baggageRestrictionManager);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright (c) 2017, Uber Technologies, Inc
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package com.uber.jaeger.baggage;

public abstract class BaggageRestrictionManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add javadocs on what this is?

static final int DEFAULT_MAX_VALUE_LENGTH = 2048;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only used in the DefaultBaggageRestrictionManager. Do you see this being made use of elsewhere? Perhaps it can be moved to DefaultBaggageRestrictionManager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it'll be used in another manager in the follow up PR


public abstract BaggageValidity isBaggageValid(String key, String value);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Copyright (c) 2017, Uber Technologies, Inc
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package com.uber.jaeger.baggage;

import com.uber.jaeger.Span;
import com.uber.jaeger.SpanContext;
import com.uber.jaeger.metrics.Metrics;
import lombok.Value;

import java.util.HashMap;
import java.util.Map;

@Value(staticConstructor = "of")
public class BaggageValidity {
final boolean valid;
final int maxValueLength;

public SpanContext sanitizeBaggage(Span span, String key, String value) {
Metrics metrics = span.getTracer().getMetrics();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd initialize the metrics object by accepting a Metrics when creating an instance of the class. There is no necessity to retrieve it once per method call.

See also: Law of Demeter

if (!this.isValid()) {
metrics.baggageUpdateFailure.inc(1);
this.logFields(span, key, value, null, false, true);
return span.context();
}
boolean truncated = false;
if (value.length() > maxValueLength) {
truncated = true;
value = value.substring(0, maxValueLength);
metrics.baggageTruncate.inc(1);
}

String prevItem = span.getBaggageItem(key);
this.logFields(span, key, value, prevItem, truncated, false);
SpanContext context = span.context().withBaggageItem(key, value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to create the new context and pass it back given the setter isn't public. Also the BaggageValidity class reeks of feature envy. I could move this file down to the same namespace as Span and make the context setter protected or I could remove the functionality in this class completely and inline it in Span.setBaggageItem()

metrics.baggageUpdateSuccess.inc(1);
return context;
}

void logFields(Span span, String key, String value, String prevItem, boolean truncated, boolean invalid) {
if (span.context().isSampled()) {
Map<String, String> fields = new HashMap<String, String>();
fields.put("event", "baggage");
fields.put("key", key);
fields.put("value", value);
if (prevItem != null) {
fields.put("override", "true");
}
if (truncated) {
fields.put("truncated", "true");
}
if (invalid) {
fields.put("invalid", "true");
}
span.log(fields);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright (c) 2017, Uber Technologies, Inc
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package com.uber.jaeger.baggage;

public class DefaultBaggageRestrictionManager extends BaggageRestrictionManager {

private final BaggageValidity baggageValidity;

public DefaultBaggageRestrictionManager() {
this(DEFAULT_MAX_VALUE_LENGTH);
}

public DefaultBaggageRestrictionManager(int maxValueLength) {
baggageValidity = BaggageValidity.of(true, maxValueLength);
}

@Override
public BaggageValidity isBaggageValid(String key, String value) {
return baggageValidity;
}
}
18 changes: 18 additions & 0 deletions jaeger-core/src/main/java/com/uber/jaeger/metrics/Metrics.java
Original file line number Diff line number Diff line change
Expand Up @@ -211,4 +211,22 @@ public static Metrics fromStatsReporter(StatsReporter reporter) {
)
// Number of times the Sampler failed to parse retrieved sampling strategy
public Counter samplerParsingFailure;

@Metric(
name = "baggage-update",
tags = {@Tag(key = "result", value = "ok")}
)
// Number of times baggage was successfully written or updated on spans
public Counter baggageUpdateSuccess;

@Metric(
name = "baggage-update",
tags = {@Tag(key = "result", value = "err")}
)
// Number of times baggage failed to write or update on spans
public Counter baggageUpdateFailure;

@Metric(name = "baggage-truncate")
// Number of times baggage was truncated as per baggage restrictions
public Counter baggageTruncate;
}
Loading