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

Add BaggageRestrictionManager #217

Merged
21 changes: 7 additions & 14 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.BaggageSetter;
import io.opentracing.tag.Tags;
import java.util.ArrayList;
import java.util.Collections;
Expand Down Expand Up @@ -116,22 +117,14 @@ public List<LogData> getLogs() {

@Override
public Span setBaggageItem(String key, String value) {
if (key == null || value == null) {
return this;
}
BaggageSetter setter = this.getTracer().getBaggageRestrictionManager().getBaggageSetter(key);
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);
}
this.context = setter.setBaggage(this, key, value);
return this;
}
return this;
}

@Override
Expand Down
26 changes: 21 additions & 5 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 @@ -442,12 +450,13 @@ public static final class Builder {
private final Sampler sampler;
private final Reporter reporter;
private final PropagationRegistry registry = new PropagationRegistry();
private Metrics metrics;
private Metrics metrics = new Metrics(new StatsFactoryImpl(new NullStatsReporter()));
private String serviceName;
private Clock clock = new SystemClock();
private Map<String, Object> tags = new HashMap<String, Object>();
private boolean zipkinSharedRpcSpan;
private ActiveSpanSource activeSpanSource = new ThreadLocalActiveSpanSource();
private BaggageRestrictionManager baggageRestrictionManager;

public Builder(String serviceName, Reporter reporter, Sampler sampler) {
if (serviceName == null || serviceName.trim().length() == 0) {
Expand All @@ -456,7 +465,6 @@ public Builder(String serviceName, Reporter reporter, Sampler sampler) {
this.serviceName = serviceName;
this.reporter = reporter;
this.sampler = sampler;
this.metrics = new Metrics(new StatsFactoryImpl(new NullStatsReporter()));

TextMapCodec textMapCodec = new TextMapCodec(false);
this.registerInjector(Format.Builtin.TEXT_MAP, textMapCodec);
Expand Down Expand Up @@ -524,9 +532,17 @@ public Builder withTags(Map<String, String> tags) {
return this;
}

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

public Tracer build() {
if (baggageRestrictionManager == null) {
baggageRestrictionManager = new DefaultBaggageRestrictionManager(this.metrics);
}
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,35 @@
/*
* 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;

/**
* BaggageRestrictionManager is an abstract class that manages baggage
* restrictions for baggage keys. The manager will return a {@link BaggageSetter}
* for a specific baggage key which will set the baggage on the {@link com.uber.jaeger.Span}
* given the baggage restrictions for that key.
*/
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 BaggageSetter getBaggageSetter(String key);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* 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 java.util.HashMap;
import java.util.Map;

import lombok.Value;

/**
* BaggageSetter is a class that sets baggage and the logs associated
* with the baggage on a given {@link Span}.
*/
@Value(staticConstructor = "of")
public class BaggageSetter {
final boolean valid;
final int maxValueLength;
final Metrics metrics;
Copy link
Member

Choose a reason for hiding this comment

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

  • to make fields visible to subclasses, they should be protected. Did you also need package visibility?
  • given that these fields are final, there should be a constructor setting them. If they are only set by subclasses, then the constructor can be protected (but then this class should be abstract).
  • the concept of valid is not clear, please document

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@value(staticConstructor = "of") generates a private constructor and a public factory for this class.


/**
* Sets the baggage key:value on the {@link Span} and the corresponding
* logs. Whether the baggage is set on the span depends on if the key
* is valid.
Copy link
Member

Choose a reason for hiding this comment

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

s/is valid/is allowed to be set by this service/

* <p>
* A {@link SpanContext} is returned with the new baggage key:value set
* if key is valid, else returns the existing {@link SpanContext}
* on the {@link Span}.
*
* @param span the span to set the baggage on
* @param key the baggage key to set
* @param value the baggage value to set
* @return the SpanContext with the baggage set
*/
public SpanContext setBaggage(Span span, String key, String value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add javadoc, simply based on the method name, I'd conclude that it simply sets baggage.

if (!valid) {
metrics.baggageUpdateFailure.inc(1);
logFields(span, key, value, null, false, true);
Copy link
Member

Choose a reason for hiding this comment

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

if you move prevItem and truncated declarations to the top, this could be made a lot more readable: logFields(span, key, value, prevValue, truncated, valid);

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);
logFields(span, key, value, prevItem, truncated, false);
SpanContext context = span.context().withBaggageItem(key, value);
metrics.baggageUpdateSuccess.inc(1);
return context;
}

private void logFields(Span span, String key, String value, String prevItem, boolean truncated, boolean invalid) {
Copy link
Member

Choose a reason for hiding this comment

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

avoid negatives - s/invalid/valid/

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");
Copy link
Member

Choose a reason for hiding this comment

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

why are you logging these as strings instead of booleans?

Copy link
Member

Choose a reason for hiding this comment

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

ah, nvm

}
span.log(fields);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* 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.metrics.Metrics;

public class DefaultBaggageRestrictionManager extends BaggageRestrictionManager {
private final BaggageSetter baggageSetter;

public DefaultBaggageRestrictionManager(Metrics metrics) {
this(metrics, DEFAULT_MAX_VALUE_LENGTH);
}

public DefaultBaggageRestrictionManager(Metrics metrics, int maxValueLength) {
baggageSetter = BaggageSetter.of(true, maxValueLength, metrics);
}

@Override
public BaggageSetter getBaggageSetter(String key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this?
Can't we use lombok's @Getter annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getBaggageSetter is an interface function

Copy link
Contributor

Choose a reason for hiding this comment

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

discussed this offline: my suggestion doesn't work because the getter requires a key

return baggageSetter;
}
}
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