Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Getter.Keys() with Jaeger Baggage support. #1549

Merged
merged 15 commits into from
Nov 5, 2020
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import io.opentelemetry.context.propagation.TextMapPropagator.Getter;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -40,6 +41,11 @@ public class HttpTraceContextExtractBenchmark {
private final HttpTraceContext httpTraceContext = HttpTraceContext.getInstance();
private final Getter<Map<String, String>> getter =
new Getter<Map<String, String>>() {
@Override
public Collection<String> keys(Map<String, String> carrier) {
return carrier.keySet();
}

@Override
public String get(Map<String, String> carrier, String key) {
return carrier.get(key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,29 @@
import io.opentelemetry.baggage.BaggageUtils;
import io.opentelemetry.baggage.EntryMetadata;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapPropagator.Getter;
import java.util.Collection;
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;

class W3CBaggagePropagatorTest {

private static final Getter<Map<String, String>> getter =
new Getter<Map<String, String>>() {
@Override
public Collection<String> keys(Map<String, String> carrier) {
return carrier.keySet();
}

@Nullable
@Override
public String get(Map<String, String> carrier, String key) {
return carrier.get(key);
}
};

@Test
void fields() {
assertThat(W3CBaggagePropagator.getInstance().fields()).containsExactly("baggage");
Expand All @@ -28,8 +45,7 @@ void fields() {
void extract_noBaggageHeader() {
W3CBaggagePropagator propagator = new W3CBaggagePropagator();

Context result =
propagator.extract(Context.root(), ImmutableMap.<String, String>of(), Map::get);
Context result = propagator.extract(Context.root(), ImmutableMap.<String, String>of(), getter);

assertThat(result).isEqualTo(Context.root());
}
Expand All @@ -38,8 +54,7 @@ void extract_noBaggageHeader() {
void extract_emptyBaggageHeader() {
W3CBaggagePropagator propagator = new W3CBaggagePropagator();

Context result =
propagator.extract(Context.root(), ImmutableMap.of("baggage", ""), ImmutableMap::get);
Context result = propagator.extract(Context.root(), ImmutableMap.of("baggage", ""), getter);

assertThat(BaggageUtils.getBaggage(result)).isEqualTo(Baggage.empty());
}
Expand All @@ -49,8 +64,7 @@ void extract_singleEntry() {
W3CBaggagePropagator propagator = new W3CBaggagePropagator();

Context result =
propagator.extract(
Context.root(), ImmutableMap.of("baggage", "key=value"), ImmutableMap::get);
propagator.extract(Context.root(), ImmutableMap.of("baggage", "key=value"), getter);

Baggage expectedBaggage = Baggage.builder().put("key", "value").build();
assertThat(BaggageUtils.getBaggage(result)).isEqualTo(expectedBaggage);
Expand All @@ -62,9 +76,7 @@ void extract_multiEntry() {

Context result =
propagator.extract(
Context.root(),
ImmutableMap.of("baggage", "key1=value1,key2=value2"),
ImmutableMap::get);
Context.root(), ImmutableMap.of("baggage", "key1=value1,key2=value2"), getter);

Baggage expectedBaggage = Baggage.builder().put("key1", "value1").put("key2", "value2").build();
assertThat(BaggageUtils.getBaggage(result)).isEqualTo(expectedBaggage);
Expand All @@ -76,7 +88,7 @@ void extract_duplicateKeys() {

Context result =
propagator.extract(
Context.root(), ImmutableMap.of("baggage", "key=value1,key=value2"), ImmutableMap::get);
Context.root(), ImmutableMap.of("baggage", "key=value1,key=value2"), getter);

Baggage expectedBaggage = Baggage.builder().put("key", "value2").build();
assertThat(BaggageUtils.getBaggage(result)).isEqualTo(expectedBaggage);
Expand All @@ -90,7 +102,7 @@ void extract_withMetadata() {
propagator.extract(
Context.root(),
ImmutableMap.of("baggage", "key=value;metadata-key=value;othermetadata"),
ImmutableMap::get);
getter);

Baggage expectedBaggage =
Baggage.builder()
Expand All @@ -110,7 +122,7 @@ void extract_fullComplexities() {
"baggage",
"key1= value1; metadata-key = value; othermetadata, "
+ "key2 =value2 , key3 =\tvalue3 ; "),
ImmutableMap::get);
getter);

Baggage expectedBaggage =
Baggage.builder()
Expand All @@ -136,7 +148,7 @@ void extract_invalidHeader() {
"baggage",
"key1= v;alsdf;-asdflkjasdf===asdlfkjadsf ,,a sdf9asdf-alue1; metadata-key = "
+ "value; othermetadata, key2 =value2 , key3 =\tvalue3 ; "),
ImmutableMap::get);
getter);

assertThat(BaggageUtils.getBaggage(result)).isEqualTo(Baggage.empty());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@
import io.opentelemetry.trace.TraceFlags;
import io.opentelemetry.trace.TraceId;
import io.opentelemetry.trace.TraceState;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;

/** Unit tests for {@link HttpTraceContext}. */
Expand All @@ -39,7 +41,19 @@ class HttpTraceContextTest {
private static final String TRACEPARENT_HEADER_NOT_SAMPLED =
"00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-00";
private static final Setter<Map<String, String>> setter = Map::put;
private static final Getter<Map<String, String>> getter = Map::get;
private static final Getter<Map<String, String>> getter =
new Getter<Map<String, String>>() {
@Override
public Collection<String> keys(Map<String, String> carrier) {
return carrier.keySet();
}

@Nullable
@Override
public String get(Map<String, String> carrier, String key) {
return carrier.get(key);
}
};
// Encoding preserves the order which is the reverse order of adding.
private static final String TRACESTATE_NOT_DEFAULT_ENCODING = "bar=baz,foo=bar";
private static final String TRACESTATE_NOT_DEFAULT_ENCODING_WITH_SPACES =
Expand Down Expand Up @@ -151,7 +165,7 @@ void extract_Nothing() {
// Context remains untouched.
assertThat(
httpTraceContext.extract(
Context.current(), Collections.<String, String>emptyMap(), Map::get))
Context.current(), Collections.<String, String>emptyMap(), getter))
.isSameAs(Context.current());
}

Expand All @@ -169,9 +183,7 @@ void extract_SampledContext() {
void extract_NullCarrier() {
Map<String, String> carrier = new LinkedHashMap<>();
carrier.put(TRACE_PARENT, TRACEPARENT_HEADER_SAMPLED);
assertThat(
getSpanContext(
httpTraceContext.extract(Context.current(), null, (c, k) -> carrier.get(k))))
assertThat(getSpanContext(httpTraceContext.extract(Context.current(), carrier, getter)))
.isEqualTo(
SpanContext.createFromRemoteParent(
TRACE_ID_BASE16, SPAN_ID_BASE16, SAMPLED_TRACE_OPTIONS, TRACE_STATE_DEFAULT));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package io.opentelemetry.context.propagation;

import io.opentelemetry.context.Context;
import java.util.Collection;
import java.util.List;
import javax.annotation.Nullable;
import javax.annotation.concurrent.ThreadSafe;
Expand Down Expand Up @@ -115,6 +116,14 @@ interface Setter<C> {
*/
interface Getter<C> {

/**
* Returns all the keys in the given carrier.
*
* @param carrier carrier of propagation fields, such as an http request.
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
* @since 0.10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we've gotten rid of all the @since tags for now; we'll set them all to 1.0 when we're ready to release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I totally forgot about it. Will do.

*/
Collection<String> keys(C carrier);
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns the first value of the given propagation {@code key} or returns {@code null}.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.context.ContextKey;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -155,6 +156,11 @@ private MapSetter() {}
private static final class MapGetter implements TextMapPropagator.Getter<Map<String, String>> {
private static final MapGetter INSTANCE = new MapGetter();

@Override
public Collection<String> keys(Map<String, String> map) {
return map.keySet();
}

@Override
public String get(Map<String, String> map, String key) {
return map.get(key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import io.opentelemetry.context.propagation.TextMapPropagator;
import io.opentelemetry.trace.Span;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -94,6 +95,11 @@ public static class JaegerContextExtractBenchmark extends AbstractContextExtract

private final TextMapPropagator.Getter<Map<String, String>> getter =
new TextMapPropagator.Getter<Map<String, String>>() {
@Override
public Collection<String> keys(Map<String, String> carrier) {
return carrier.keySet();
}

@Override
public String get(Map<String, String> carrier, String key) {
return carrier.get(key);
Expand Down Expand Up @@ -137,6 +143,11 @@ public static class JaegerUrlEncodedContextExtractBenchmark

private final TextMapPropagator.Getter<Map<String, String>> getter =
new TextMapPropagator.Getter<Map<String, String>>() {
@Override
public Collection<String> keys(Map<String, String> carrier) {
return carrier.keySet();
}

@Override
public String get(Map<String, String> carrier, String key) {
return carrier.get(key);
Expand Down Expand Up @@ -180,6 +191,11 @@ public static class B3SingleHeaderContextExtractBenchmark

private final TextMapPropagator.Getter<Map<String, String>> getter =
new TextMapPropagator.Getter<Map<String, String>>() {
@Override
public Collection<String> keys(Map<String, String> carrier) {
return carrier.keySet();
}

@Override
public String get(Map<String, String> carrier, String key) {
return carrier.get(key);
Expand Down Expand Up @@ -226,6 +242,11 @@ private static Map<String, String> createHeaders(

private final TextMapPropagator.Getter<Map<String, String>> getter =
new TextMapPropagator.Getter<Map<String, String>>() {
@Override
public Collection<String> keys(Map<String, String> carrier) {
return carrier.keySet();
}

@Override
public String get(Map<String, String> carrier, String key) {
return carrier.get(key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@

package io.opentelemetry.extensions.trace.propagation;

import io.opentelemetry.baggage.Baggage;
import io.opentelemetry.baggage.BaggageUtils;
import io.opentelemetry.baggage.Entry;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapPropagator;
import io.opentelemetry.trace.Span;
Expand Down Expand Up @@ -34,6 +37,8 @@ public class JaegerPropagator implements TextMapPropagator {
private static final Logger logger = Logger.getLogger(JaegerPropagator.class.getName());

static final String PROPAGATION_HEADER = "uber-trace-id";
static final String BAGGAGE_HEADER = "jaeger-baggage";
static final String BAGGAGE_PREFIX = "uberctx-";
// Parent span has been deprecated but Jaeger propagation protocol requires it
static final char DEPRECATED_PARENT_SPAN = '0';
static final char PROPAGATION_HEADER_DELIMITER = ':';
Expand Down Expand Up @@ -84,10 +89,15 @@ public <C> void inject(Context context, C carrier, Setter<C> setter) {
Objects.requireNonNull(setter, "setter");

SpanContext spanContext = Span.fromContext(context).getSpanContext();
if (!spanContext.isValid()) {
return;
if (spanContext.isValid()) {
injectSpan(spanContext, carrier, setter);
}

injectBaggage(BaggageUtils.getBaggage(context), carrier, setter);
}

private static <C> void injectSpan(SpanContext spanContext, C carrier, Setter<C> setter) {

char[] chars = new char[PROPAGATION_HEADER_SIZE];

String traceId = spanContext.getTraceIdAsHexString();
Expand All @@ -108,16 +118,27 @@ public <C> void inject(Context context, C carrier, Setter<C> setter) {
setter.set(carrier, PROPAGATION_HEADER, new String(chars));
}

private static <C> void injectBaggage(Baggage baggage, C carrier, Setter<C> setter) {
for (Entry entry : baggage.getEntries()) {
setter.set(carrier, BAGGAGE_PREFIX + entry.getKey(), entry.getValue());
}
}

@Override
public <C> Context extract(Context context, @Nullable C carrier, Getter<C> getter) {
Objects.requireNonNull(getter, "getter");

SpanContext spanContext = getSpanContextFromHeader(carrier, getter);
if (!spanContext.isValid()) {
return context;
if (spanContext.isValid()) {
context = context.with(Span.wrap(spanContext));
}

Baggage baggage = getBaggageFromHeader(carrier, getter);
if (baggage != null) {
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
context = context.with(baggage);
}

return context.with(Span.wrap(spanContext));
return context;
}

@SuppressWarnings("StringSplitter")
Expand Down Expand Up @@ -187,6 +208,42 @@ private static <C> SpanContext getSpanContextFromHeader(C carrier, Getter<C> get
return buildSpanContext(traceId, spanId, flags);
}

private static <C> Baggage getBaggageFromHeader(C carrier, Getter<C> getter) {
Baggage.Builder builder = null;

for (String key : getter.keys(carrier)) {
if (key.startsWith(BAGGAGE_PREFIX)) {
if (key.length() <= BAGGAGE_PREFIX.length()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we already checked startsWith I think this can be equals check

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'd rather stick to check the length, as it only depends on a numerical comparison.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I meant an equal check on the length - if it starts with it, then the length is guarateed to be at least that length and the < here looks redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

continue;
}

if (builder == null) {
builder = Baggage.builder();
}
builder.put(key.substring(BAGGAGE_PREFIX.length()), getter.get(carrier, key));
} else if (key.equals(BAGGAGE_HEADER)) {
builder = parseBaggageHeader(getter.get(carrier, key), builder);
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
}
}
return builder == null ? null : builder.build();
}

@SuppressWarnings("StringSplitter")
Copy link
Contributor

Choose a reason for hiding this comment

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

not relevant for this PR in particular, but I think we should turn off this warning globally, since it's telling us to use guava, which we don't to do for the API, and won't, ever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

private static Baggage.Builder parseBaggageHeader(String header, Baggage.Builder builder) {
for (String part : header.split("\\s*,\\s*")) {
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
String[] kv = part.split("\\s*=\\s*");
if (kv.length == 2) {
if (builder == null) {
builder = Baggage.builder();
}
builder.put(kv[0], kv[1]);
} else {
logger.fine("malformed token in " + BAGGAGE_HEADER + " header: " + part);
}
}
return builder;
}

private static SpanContext buildSpanContext(String traceId, String spanId, String flags) {
try {
int flagsInt = Integer.parseInt(flags);
Expand Down
Loading