From 733a58600e2ebea1c124b0fd16fda48b275b4fc3 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 7 May 2020 14:28:11 +0800 Subject: [PATCH] Onto latest brave --- .../TraceContextPropagationBenchmarks.java | 2 +- .../w3c/TraceContextExtractor.java | 57 ++--- .../propagation/w3c/TraceContextInjector.java | 37 ++-- .../w3c/TraceContextPropagation.java | 79 +++---- .../propagation/w3c/TraceparentFormat.java | 2 +- .../brave/propagation/w3c/Tracestate.java | 64 ++++-- .../propagation/w3c/TracestateFormat.java | 203 +++++------------- ...raceContextPropagationClassLoaderTest.java | 10 +- .../w3c/TraceContextPropagationTest.java | 57 ++--- .../propagation/w3c/TracestateFormatTest.java | 82 +++---- 10 files changed, 225 insertions(+), 368 deletions(-) diff --git a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java index d4ff67b138..659c020171 100644 --- a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java @@ -13,7 +13,7 @@ */ package brave.propagation.w3c; -import brave.internal.HexCodec; +import brave.internal.codec.HexCodec; import brave.propagation.Propagation; import brave.propagation.TraceContext; import brave.propagation.TraceContext.Extractor; diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java index 6b2910d4d2..5a5cb89997 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java @@ -19,31 +19,26 @@ import brave.propagation.TraceContextOrSamplingFlags; import static brave.propagation.B3SingleFormat.parseB3SingleFormat; +import static brave.propagation.w3c.TraceContextPropagation.TRACEPARENT; +import static brave.propagation.w3c.TraceContextPropagation.TRACESTATE; // TODO: this class has no useful tests wrt traceparent yet -final class TraceContextExtractor implements Extractor { - static final TraceContextOrSamplingFlags EXTRACTED_EMPTY = - TraceContextOrSamplingFlags.EMPTY.toBuilder().addExtra(Tracestate.EMPTY).build(); +final class TraceContextExtractor implements Extractor { + final Getter getter; + final TraceContextPropagation propagation; - final Getter getter; - final K traceparentKey, tracestateKey; - final TracestateFormat tracestateFormat; - final B3SingleFormatHandler handler = new B3SingleFormatHandler(); - - TraceContextExtractor(TraceContextPropagation propagation, Getter getter) { + TraceContextExtractor(TraceContextPropagation propagation, Getter getter) { this.getter = getter; - this.traceparentKey = propagation.traceparent; - this.tracestateKey = propagation.tracestate; - this.tracestateFormat = new TracestateFormat(propagation.tracestateKey); + this.propagation = propagation; } @Override public TraceContextOrSamplingFlags extract(R request) { if (request == null) throw new NullPointerException("request == null"); - String traceparentString = getter.get(request, traceparentKey); - if (traceparentString == null) return EXTRACTED_EMPTY; + String traceparentString = getter.get(request, TRACEPARENT); + if (traceparentString == null) return TraceContextOrSamplingFlags.EMPTY; // TODO: add link that says tracestate itself is optional - String tracestateString = getter.get(request, tracestateKey); + String tracestateString = getter.get(request, TRACESTATE); if (tracestateString == null) { // NOTE: we may not want to pay attention to the sampled flag. Since it conflates // not-yet-sampled with sampled=false, implementations that always set flags to -00 would @@ -54,29 +49,19 @@ final class TraceContextExtractor implements Extractor { // read the tracestate header. Trusting the span ID (traceparent calls the span ID parent-id) // could result in a headless trace. TraceContext maybeUpstream = TraceparentFormat.parseTraceparentFormat(traceparentString); - return TraceContextOrSamplingFlags.newBuilder(maybeUpstream) - .addExtra(Tracestate.EMPTY) // marker for outbound propagation - .build(); - } - - Tracestate tracestate = tracestateFormat.parseAndReturnOtherEntries(tracestateString, handler); - - TraceContext context = handler.context; - if (context == null) { - if (tracestate == Tracestate.EMPTY) return EXTRACTED_EMPTY; - return EXTRACTED_EMPTY.toBuilder().addExtra(tracestate).build(); + return TraceContextOrSamplingFlags.create(maybeUpstream); } - return TraceContextOrSamplingFlags.newBuilder(context).addExtra(tracestate).build(); - } - - static final class B3SingleFormatHandler implements TracestateFormat.Handler { - TraceContext context; - @Override - public boolean onThisEntry(CharSequence tracestate, int beginIndex, int endIndex) { - TraceContextOrSamplingFlags extracted = parseB3SingleFormat(tracestate, beginIndex, endIndex); - if (extracted != null) context = extracted.context(); - return context != null; + Tracestate tracestate = propagation.tracestateFactory.create(); + TraceContextOrSamplingFlags extracted = null; + if (TracestateFormat.INSTANCE.parseInto(tracestateString, tracestate)) { + String b3 = tracestate.get(propagation.tracestateKey); + if (b3 != null) { + tracestate.put(propagation.tracestateKey, null); + extracted = parseB3SingleFormat(b3); + } } + if (extracted == null) extracted = TraceContextOrSamplingFlags.EMPTY; + return extracted.toBuilder().addExtra(tracestate).build(); } } diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java index 2d9d0fe21e..bf81025d85 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java @@ -13,39 +13,28 @@ */ package brave.propagation.w3c; +import brave.propagation.B3SingleFormat; import brave.propagation.Propagation.Setter; import brave.propagation.TraceContext; import brave.propagation.TraceContext.Injector; -import static brave.propagation.B3SingleFormat.writeB3SingleFormat; +import static brave.propagation.w3c.TraceContextPropagation.TRACEPARENT; +import static brave.propagation.w3c.TraceContextPropagation.TRACESTATE; import static brave.propagation.w3c.TraceparentFormat.writeTraceparentFormat; -final class TraceContextInjector implements Injector { - final TracestateFormat tracestateFormat; - final Setter setter; - final K traceparentKey, tracestateKey; +final class TraceContextInjector implements Injector { + final Setter setter; + final String tracestateKey; - TraceContextInjector(TraceContextPropagation propagation, Setter setter) { - this.tracestateFormat = new TracestateFormat(propagation.tracestateKey); - this.traceparentKey = propagation.traceparent; - this.tracestateKey = propagation.tracestate; + TraceContextInjector(TraceContextPropagation propagation, Setter setter) { this.setter = setter; + this.tracestateKey = propagation.tracestateKey; } - @Override public void inject(TraceContext traceContext, R request) { - - setter.put(request, traceparentKey, writeTraceparentFormat(traceContext)); - - CharSequence otherState = null; - for (int i = 0, length = traceContext.extra().size(); i < length; i++) { - Object next = traceContext.extra().get(i); - if (next instanceof Tracestate) { - otherState = ((Tracestate) next).otherEntries; - break; - } - } - - String tracestate = tracestateFormat.write(writeB3SingleFormat(traceContext), otherState); - setter.put(request, tracestateKey, tracestate); + @Override public void inject(TraceContext context, R request) { + setter.put(request, TRACEPARENT, writeTraceparentFormat(context)); + Tracestate tracestate = context.findExtra(Tracestate.class); + tracestate.put(tracestateKey, B3SingleFormat.writeB3SingleFormat(context)); + setter.put(request, TRACESTATE, tracestate.stateString()); } } diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java index 84c4c32f0e..2be7c177c8 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java @@ -13,23 +13,30 @@ */ package brave.propagation.w3c; +import brave.internal.propagation.StringPropagationAdapter; import brave.propagation.Propagation; import brave.propagation.TraceContext; import brave.propagation.TraceContext.Extractor; import brave.propagation.TraceContext.Injector; -import java.util.Arrays; +import java.util.Collections; import java.util.List; -public final class TraceContextPropagation implements Propagation { - public static Factory newFactory() { - return new FactoryBuilder().build(); +import static java.util.Arrays.asList; + +public final class TraceContextPropagation extends Propagation.Factory + implements Propagation { + static final String TRACEPARENT = "traceparent", TRACESTATE = "tracestate"; + + public static Propagation.Factory create() { + return new Builder().build(); } - public static FactoryBuilder newFactoryBuilder() { - return new FactoryBuilder(); + public static Builder newBuilder() { + return new Builder(); } - public static final class FactoryBuilder { + public static final class Builder { + static final TracestateFormat THROWING_VALIDATOR = new TracestateFormat(true); String tracestateKey = "b3"; /** @@ -38,60 +45,56 @@ public static final class FactoryBuilder { * @throws IllegalArgumentException if the key doesn't conform to ABNF rules defined by the * trace-context specification. */ - public FactoryBuilder tracestateKey(String key) { + public Builder tracestateKey(String key) { if (key == null) throw new NullPointerException("key == null"); - TracestateFormat.validateKey(key, true); + THROWING_VALIDATOR.validateKey(key, 0, key.length()); this.tracestateKey = key; return this; } - public Factory build() { - return new Factory(this); + public Propagation.Factory build() { + return new TraceContextPropagation(this); } - FactoryBuilder() { + Builder() { } } - static final class Factory extends Propagation.Factory { - final String tracestateKey; + final String tracestateKey; + final Tracestate.Factory tracestateFactory; + final List keys = Collections.unmodifiableList(asList(TRACEPARENT, TRACESTATE)); - Factory(FactoryBuilder builder) { - this.tracestateKey = builder.tracestateKey; - } + TraceContextPropagation(Builder builder) { + this.tracestateKey = builder.tracestateKey; + this.tracestateFactory = Tracestate.newFactory(tracestateKey); + } - @Override public Propagation create(KeyFactory keyFactory) { - return new TraceContextPropagation<>(keyFactory, tracestateKey); - } + @Override public List keys() { + return keys; + } - @Override public TraceContext decorate(TraceContext context) { - // TODO: almost certain we will need to decorate as not all contexts will start with an - // incoming request (ex schedule or client-originated traces) - return super.decorate(context); - } + @Override public boolean requires128BitTraceId() { + return true; } - final String tracestateKey; - final K traceparent, tracestate; - final List keys; - - TraceContextPropagation(KeyFactory keyFactory, String tracestateKey) { - this.tracestateKey = tracestateKey; - this.traceparent = keyFactory.create("traceparent"); - this.tracestate = keyFactory.create("tracestate"); - this.keys = Arrays.asList(traceparent, tracestate); + @Override public TraceContext decorate(TraceContext context) { + return tracestateFactory.decorate(context); } - @Override public List keys() { - return keys; + @Override public Propagation get() { + return this; + } + + @Override public Propagation create(KeyFactory keyFactory) { + return StringPropagationAdapter.create(this, keyFactory); } - @Override public Injector injector(Setter setter) { + @Override public Injector injector(Setter setter) { if (setter == null) throw new NullPointerException("setter == null"); return new TraceContextInjector<>(this, setter); } - @Override public Extractor extractor(Getter getter) { + @Override public Extractor extractor(Getter getter) { if (getter == null) throw new NullPointerException("getter == null"); return new TraceContextExtractor<>(this, getter); } diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java index 2aabf11077..063d0a767a 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java @@ -18,7 +18,7 @@ import brave.propagation.TraceContext; import java.nio.ByteBuffer; -import static brave.internal.HexCodec.writeHexLong; +import static brave.internal.codec.HexCodec.writeHexLong; /** Implements https://w3c.github.io/trace-context/#traceparent-header */ // TODO: this uses the internal Platform class as it defers access to the logger and makes JUL less diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java b/propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java index 63125fe247..b97afe7dcc 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java @@ -13,38 +13,56 @@ */ package brave.propagation.w3c; -import brave.internal.Nullable; +import brave.internal.extra.MapExtra; +import brave.internal.extra.MapExtraFactory; -/** - * This only contains other entries. The entry for the current trace is only written during - * injection. - */ -final class Tracestate { // hidden intentionally - static final Tracestate EMPTY = new Tracestate(""); +final class Tracestate extends MapExtra { + static Factory newFactory(String tracestateKey) { + // max is total initial + dynamic + return new FactoryBuilder().addInitialKey(tracestateKey).maxDynamicEntries(31).build(); + } + + static final class FactoryBuilder extends + MapExtraFactory.Builder { + @Override protected Factory build() { + return new Factory(this); + } + } - // TODO: this will change - final String otherEntries; + static final class Factory extends MapExtraFactory { + Factory(FactoryBuilder builder) { + super(builder); + } - static Tracestate create(@Nullable CharSequence otherEntries) { - if (otherEntries == null || otherEntries.length() == 0) return EMPTY; - return new Tracestate(otherEntries.toString()); + @Override protected Tracestate create() { + return new Tracestate(this); + } } - private Tracestate(String otherEntries) { - this.otherEntries = otherEntries; + Tracestate(Factory factory) { + super(factory); } - @Override public String toString() { - return "tracestate: " + otherEntries; + @Override protected String get(String key) { + return super.get(key); } - @Override public final boolean equals(Object o) { - if (o == this) return true; - if (!(o instanceof Tracestate)) return false; - return otherEntries.equals(((Tracestate) o).otherEntries); + @Override protected String stateString() { + Object[] array = (Object[]) state; + // TODO: SHOULD on 512 char limit https://w3c.github.io/trace-context/#tracestate-limits + StringBuilder result = new StringBuilder(); + boolean empty = true; + for (int i = 0; i < array.length; i += 2) { + String key = (String) array[i], value = (String) array[i + 1]; + if (value == null) continue; + if (!empty) result.append(','); + result.append(key).append('=').append(value); + empty = false; + } + return result.toString(); } - @Override public final int hashCode() { - return otherEntries.hashCode(); + @Override protected boolean put(String key, String value) { + return super.put(key, value); } -} \ No newline at end of file +} diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java index 58d1a72ba4..802690fc01 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java @@ -14,8 +14,7 @@ package brave.propagation.w3c; import brave.internal.Platform; - -import static brave.propagation.w3c.TraceparentFormat.FORMAT_LENGTH; +import brave.internal.codec.EntrySplitter; /** * Implements https://w3c.github.io/trace-context/#tracestate-header @@ -25,107 +24,25 @@ * specific. We choose to not use the term vendor as this is open source code. Instead, we use term * entry (key/value). */ -final class TracestateFormat { - final String key; - final int keyLength; - final int entryLength; - - TracestateFormat(String key) { - this.key = key; - this.keyLength = key.length(); - this.entryLength = keyLength + 1 /* = */ + FORMAT_LENGTH; - } - - enum Op { - THIS_ENTRY, - OTHER_ENTRIES - } - - interface Handler { - boolean onThisEntry(CharSequence tracestate, int beginIndex, int endIndex); - } - - // TODO: SHOULD on 512 char limit https://w3c.github.io/trace-context/#tracestate-limits - String write(String thisValue, CharSequence otherEntries) { - int extraLength = otherEntries == null ? 0 : otherEntries.length(); - - char[] result; - if (extraLength == 0) { - result = new char[entryLength]; - } else { - result = new char[entryLength + 1 /* , */ + extraLength]; - } - - int pos = 0; - for (int i = 0; i < keyLength; i++) { - result[pos++] = key.charAt(i); - } - result[pos++] = '='; - - for (int i = 0, len = thisValue.length(); i < len; i++) { - result[pos++] = thisValue.charAt(i); - } - - if (extraLength > 0) { // Append others after ours - result[pos++] = ','; - for (int i = 0; i < extraLength; i++) { - result[pos++] = otherEntries.charAt(i); - } - } - return new String(result, 0, pos); +final class TracestateFormat implements EntrySplitter.Handler { + static final TracestateFormat INSTANCE = new TracestateFormat(false); + + final boolean shouldThrow; + final EntrySplitter entrySplitter; + + TracestateFormat(boolean shouldThrow) { + this.shouldThrow = shouldThrow; + entrySplitter = EntrySplitter.newBuilder() + .maxEntries(32) // https://w3c.github.io/trace-context/#tracestate-header-field-values + .entrySeparator(',') + .trimOWSAroundEntrySeparator(true) // https://w3c.github.io/trace-context/#list + .keyValueSeparator('=') + .trimOWSAroundKeyValueSeparator(false) // https://github.com/w3c/trace-context/issues/409 + .shouldThrow(shouldThrow) + .build(); } - // TODO: characters were added to the valid list, so it is possible this impl no longer works - // TODO: 32 max entries https://w3c.github.io/trace-context/#tracestate-header-field-values - // TODO: empty and whitespace-only allowed Ex. 'foo=' or 'foo= ' - Tracestate parseAndReturnOtherEntries(String tracestate, Handler handler) { - StringBuilder currentString = new StringBuilder(), otherEntries = null; - Op op; - OUTER: - for (int i = 0, length = tracestate.length(); i < length; i++) { - char c = tracestate.charAt(i); - // OWS is zero or more spaces or tabs https://httpwg.org/specs/rfc7230.html#rfc.section.3.2 - if (c == ' ' || c == '\t') continue; // trim whitespace - if (c == '=') { // we reached a field name - if (++i == length) break; // skip '=' character - if (currentString.indexOf(key) == 0) { - op = Op.THIS_ENTRY; - } else { - op = Op.OTHER_ENTRIES; - if (otherEntries == null) otherEntries = new StringBuilder(); - otherEntries.append(',').append(currentString); - } - currentString.setLength(0); - } else { - currentString.append(c); - continue; - } - // no longer whitespace - switch (op) { - case OTHER_ENTRIES: - otherEntries.append(c); - while (i < length && (c = tracestate.charAt(i)) != ',') { - otherEntries.append(c); - i++; - } - break; - case THIS_ENTRY: - int nextComma = tracestate.indexOf(',', i); - int endIndex = nextComma != -1 ? nextComma : length; - if (!handler.onThisEntry(tracestate, i, endIndex)) { - break OUTER; - } - i = endIndex; - break; - } - } - if (otherEntries != null && otherEntries.charAt(0) == ',') { - otherEntries.deleteCharAt(0); // TODO: fix the parser so this is eaten before now - } - return Tracestate.create(otherEntries); - } - - // Simplify other rules by allowing value-based lookup on an ASCII value. + // Simplify parsing rules by allowing value-based lookup on an ASCII value. // // This approach is similar to io.netty.util.internal.StringUtil.HEX2B as it uses an array to // cache values. Unlike HEX2B, this requires a bounds check when using the character's integer @@ -143,67 +60,55 @@ Tracestate parseAndReturnOtherEntries(String tracestate, Handler handler) { } static boolean isValidTracestateKeyChar(char c) { - return (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') - || c == '@' || c == '_' || c == '-' || c == '*' || c == '/'; + return isLetterOrNumber(c) || c == '@' || c == '_' || c == '-' || c == '*' || c == '/'; + } + + static boolean isLetterOrNumber(char c) { + return (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9'); + } + + @Override + public boolean onEntry( + Tracestate target, String buffer, int beginKey, int endKey, int beginValue, int endValue) { + if (!validateKey(buffer, beginKey, endKey)) return false; + if (!validateValue(buffer, beginValue, beginValue)) return false; + return target.put(buffer.substring(beginKey, endKey), buffer.substring(beginValue, endValue)); + } + + boolean parseInto(String tracestateString, Tracestate tracestate) { + return entrySplitter.parse(this, tracestate, tracestateString); } /** - * Performs validation according to the ABNF of the {code tracestate} key. + * Performs validation according to the ABNF of the {@code tracestate} key. * *

See https://www.w3.org/TR/trace-context-1/#key - * - * @param shouldThrow true raises an {@link IllegalArgumentException} when validation fails - * instead of logging. */ - // The intent is to optimize for valid, non-tenant formats. Logic to narrow error messages is - // intentionally deferred. Performance matters as this could be called up to 32 times per header. - static boolean validateKey(CharSequence key, boolean shouldThrow) { - int length = key.length(); - if (length == 0) return logOrThrow("Invalid input: empty", shouldThrow); - if (length > 256) return logOrThrow("Invalid input: too large", shouldThrow); + // Logic to narrow error messages is intentionally deferred. + // Performance matters as this could be called up to 32 times per header. + boolean validateKey(CharSequence buffer, int beginKey, int endKey) { + int length = endKey - beginKey; + if (length == 0) return logOrThrow("Invalid key: empty", shouldThrow); + if (length > 256) return logOrThrow("Invalid key: too large", shouldThrow); + char first = buffer.charAt(beginKey); + if (!isLetterOrNumber(first)) { + return logOrThrow("Invalid key: must start with a-z 0-9", shouldThrow); + } - // Until we scan the entire key, we can't validate the first character, because the rules are - // different depending on if there is an '@' or not. When there's an '@', it is Tenant syntax. - int atIndex = -1; - for (int i = 0; i < length; i++) { - char c = key.charAt(i); + for (int i = beginKey + 1; i < endKey; i++) { + char c = buffer.charAt(i); if (c > LAST_VALID_KEY_CHAR || !VALID_KEY_CHARS[c]) { - return logOrThrow("Invalid input: valid characters are: a-z 0-9 _ - * / @", shouldThrow); - } - - if (c == '@') { - if (atIndex != -1) return logOrThrow("Invalid input: only one @ is allowed", shouldThrow); - atIndex = i; + return logOrThrow("Invalid key: valid characters are: a-z 0-9 _ - * / @", shouldThrow); } } - - // Now, go back and check to see if this was a Tenant formatted key, as the rules are different. - // Either way, we already checked the boundary cases. - char first = key.charAt(0); - if (atIndex == -1) return validateVendorPrefix(first, shouldThrow); - - // Unlike vendor, tenant ID can start with a number. - if ((first >= '0' && first <= '9') || first >= 'a') { // && <= 'z' implied - int vendorIndex = atIndex + 1; - int vendorLength = length - vendorIndex; - - if (vendorLength == 0) return logOrThrow("Invalid input: empty vendor", shouldThrow); - if (vendorLength > 14) return logOrThrow("Invalid input: vendor too long", shouldThrow); - - return validateVendorPrefix(key.charAt(vendorIndex), shouldThrow); - } else if (atIndex == 0) { - return logOrThrow("Invalid input: empty tenant ID", shouldThrow); - } else { - return logOrThrow("Invalid input: tenant ID must start with a-z", shouldThrow); - } + return true; } - static boolean validateVendorPrefix(char first, boolean shouldThrow) { - if (first >= 'a') { // && <= 'z' implied - return true; - } - return logOrThrow("Invalid input: vendor must start with a-z", shouldThrow); + boolean validateValue(CharSequence buffer, int beginValue, int endValue) { + // TODO: empty and whitespace-only allowed Ex. 'foo=' or 'foo= ' + // There are likely other rules, so figure out what they are and implement. + return true; } static boolean logOrThrow(String msg, boolean shouldThrow) { diff --git a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java index f829e04609..565cd0dee5 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java @@ -21,7 +21,6 @@ import java.util.Map; import org.junit.Test; -import static brave.propagation.Propagation.KeyFactory.STRING; import static brave.test.util.ClassLoaders.assertRunIsUnloadable; public class TraceContextPropagationClassLoaderTest { @@ -31,11 +30,12 @@ public class TraceContextPropagationClassLoaderTest { static class BasicUsage implements Runnable { @Override public void run() { - Propagation propagation = TraceContextPropagation.newFactory().create(STRING); - Injector> injector = propagation.injector(Map::put); - Extractor> extractor = propagation.extractor(Map::get); + Propagation.Factory propagation = TraceContextPropagation.create(); + Injector> injector = propagation.get().injector(Map::put); + Extractor> extractor = propagation.get().extractor(Map::get); - TraceContext context = TraceContext.newBuilder().traceId(1L).spanId(2L).build(); + TraceContext context = + propagation.decorate(TraceContext.newBuilder().traceId(1L).spanId(2L).build()); Map headers = new LinkedHashMap<>(); injector.inject(context, headers); diff --git a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java index 47eff3eeb1..9b3e1e0ec4 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java @@ -22,15 +22,14 @@ import java.util.Map; import org.junit.Test; -import static brave.internal.HexCodec.lowerHexToUnsignedLong; -import static brave.propagation.Propagation.KeyFactory.STRING; +import static brave.internal.codec.HexCodec.lowerHexToUnsignedLong; import static org.assertj.core.api.Assertions.assertThat; public class TraceContextPropagationTest { Map request = new LinkedHashMap<>(); - Propagation propagation = TraceContextPropagation.newFactory().create(STRING); - Injector> injector = propagation.injector(Map::put); - Extractor> extractor = propagation.extractor(Map::get); + Propagation.Factory propagation = TraceContextPropagation.create(); + Injector> injector = propagation.get().injector(Map::put); + Extractor> extractor = propagation.get().extractor(Map::get); TraceContext sampledContext = TraceContext.newBuilder() .traceIdHigh(lowerHexToUnsignedLong("67891233abcdef01")) @@ -40,10 +39,10 @@ public class TraceContextPropagationTest { .build(); String validTraceparent = "00-67891233abcdef012345678912345678-463ac35c9f6413ad-01"; String validB3Single = "67891233abcdef012345678912345678-463ac35c9f6413ad-1"; - String otherState = "congo=lZWRzIHRoNhcm5hbCBwbGVhc3VyZS4="; + String otherState = "congo=t61rcWkgMzE"; @Test public void injects_b3_when_no_other_tracestate() { - sampledContext = sampledContext.toBuilder().addExtra(Tracestate.EMPTY).build(); + sampledContext = propagation.decorate(sampledContext); injector.inject(sampledContext, request); @@ -51,9 +50,8 @@ public class TraceContextPropagationTest { } @Test public void injects_b3_before_other_tracestate() { - Tracestate tracestate = Tracestate.create(otherState); - - sampledContext = sampledContext.toBuilder().addExtra(tracestate).build(); + sampledContext = propagation.decorate(sampledContext); + TracestateFormat.INSTANCE.parseInto(otherState, sampledContext.findExtra(Tracestate.class)); injector.inject(sampledContext, request); @@ -65,17 +63,18 @@ public class TraceContextPropagationTest { request.put("tracestate", "b3=" + validB3Single); assertThat(extractor.extract(request)).isEqualTo( - TraceContextOrSamplingFlags.newBuilder(sampledContext).addExtra(Tracestate.EMPTY).build()); + TraceContextOrSamplingFlags.create(propagation.decorate(sampledContext))); } @Test public void extracts_b3_before_other_tracestate() { request.put("traceparent", validTraceparent); request.put("tracestate", "b3=" + validB3Single + "," + otherState); - Tracestate tracestate = Tracestate.create(otherState); + sampledContext = propagation.decorate(sampledContext); + TracestateFormat.INSTANCE.parseInto(otherState, sampledContext.findExtra(Tracestate.class)); - assertThat(extractor.extract(request)).isEqualTo( - TraceContextOrSamplingFlags.newBuilder(sampledContext).addExtra(tracestate).build()); + assertThat(extractor.extract(request)) + .isEqualTo(TraceContextOrSamplingFlags.create(sampledContext)); } @Test public void extracted_toString() { @@ -86,7 +85,7 @@ public class TraceContextPropagationTest { "Extracted{" + "traceContext=" + sampledContext + ", " + "samplingFlags=SAMPLED_REMOTE, " - + "extra=[tracestate: " + otherState + "]" + + "extra=[Tracestate{" + otherState + "}]" + "}"); } @@ -94,30 +93,10 @@ public class TraceContextPropagationTest { request.put("traceparent", validTraceparent); request.put("tracestate", otherState + ",b3=" + validB3Single); - Tracestate tracestate = Tracestate.create(otherState); - - assertThat(extractor.extract(request)).isEqualTo( - TraceContextOrSamplingFlags.newBuilder(sampledContext).addExtra(tracestate).build()); - } + sampledContext = propagation.decorate(sampledContext); + TracestateFormat.INSTANCE.parseInto(otherState, sampledContext.findExtra(Tracestate.class)); - @Test public void tracestate() { - Tracestate b3_withContext = Tracestate.create("b3=" + validB3Single); - Tracestate sameState = Tracestate.create("b3=" + validB3Single); - assertThat(b3_withContext).isEqualTo(sameState); - assertThat(sameState).isEqualTo(b3_withContext); - assertThat(b3_withContext).hasSameHashCodeAs(sameState); - assertThat(b3_withContext) - .hasToString("tracestate: b3=67891233abcdef012345678912345678-463ac35c9f6413ad-1"); - - assertThat(Tracestate.create("")) - .isNotEqualTo(b3_withContext) - .isSameAs(Tracestate.create(null)) - .isSameAs(Tracestate.EMPTY) - .hasToString("tracestate: "); - - Tracestate b3_debugOnly = Tracestate.create("b3=d"); - assertThat(b3_withContext).isNotEqualTo(b3_debugOnly); - assertThat(b3_debugOnly).isNotEqualTo(b3_withContext); - assertThat(b3_withContext.hashCode()).isNotEqualTo(b3_debugOnly.hashCode()); + assertThat(extractor.extract(request)) + .isEqualTo(TraceContextOrSamplingFlags.create(sampledContext)); } } diff --git a/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java b/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java index b31cccc599..12b16de263 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java @@ -14,7 +14,6 @@ package brave.propagation.w3c; import java.util.Arrays; -import java.util.stream.Stream; import org.assertj.core.api.AbstractBooleanAssert; import org.assertj.core.api.AbstractThrowableAssert; import org.junit.Test; @@ -34,26 +33,31 @@ public class TracestateFormatTest { static final String LONGEST_TENANT_KEY = "1" + TWO_HUNDRED_FORTY_KEY_CHARS + "@" + FORTY_KEY_CHARS.substring(0, 13); + TracestateFormat tracestateFormat = new TracestateFormat(true); + // all these need log assertions @Test public void validateKey_empty() { assertThatThrownByValidateKey("") - .hasMessage("Invalid input: empty"); + .hasMessage("Invalid key: empty"); } @Test public void validateKey_tooLong() { char[] tooMany = new char[257]; Arrays.fill(tooMany, 'a'); assertThatThrownByValidateKey(new String(tooMany)) - .hasMessage("Invalid input: too large"); - } - - @Test public void validateKey_shortest_basic() { - assertThatValidateKey("z").isTrue(); + .hasMessage("Invalid key: too large"); } - @Test public void validateKey_shortest_tenant() { - assertThatValidateKey("0@z").isTrue(); - assertThatValidateKey("a@z").isTrue(); + @Test public void validateKey_specialCharacters() { + for (char allowedSpecial : Arrays.asList('@', '_', '-', '*', '/')) { + assertThatThrownByValidateKey(allowedSpecial + "") + .hasMessage("Invalid key: must start with a-z 0-9"); + assertThatValidateKey("a" + allowedSpecial).isTrue(); + // Any number of special characters are allowed. ex "a*******", "a@@@@@@@" + // https://github.com/w3c/trace-context/pull/386 + assertThatValidateKey("a" + allowedSpecial + allowedSpecial).isTrue(); + assertThatValidateKey("a" + allowedSpecial + "1").isTrue(); + } } @Test public void validateKey_longest_basic() { @@ -64,54 +68,28 @@ public class TracestateFormatTest { assertThatValidateKey(LONGEST_TENANT_KEY).isTrue(); } - @Test public void validateKey_invalid_basic() { - // zero is allowed only as when there is an '@' - assertThatThrownByValidateKey("0") - .hasMessage("Invalid input: vendor must start with a-z"); - } - - @Test public void validateKey_invalid_basic_unicode() { - Stream.of("a💩", "💩a").forEach(key -> assertThatThrownByValidateKey(key) - .hasMessage("Invalid input: valid characters are: a-z 0-9 _ - * / @")); - } - - @Test public void validateKey_invalid_tenant() { - assertThatThrownByValidateKey("_@z") - .hasMessage("Invalid input: tenant ID must start with a-z"); - } - - @Test public void validateKey_invalid_tenant_unicode() { - Stream.of( - "a@a💩", - "a@💩a", - "a💩@a", - "💩a@a" - ).forEach(key -> assertThatThrownByValidateKey(key) - .hasMessage("Invalid input: valid characters are: a-z 0-9 _ - * / @")); - } - - @Test public void validateKey_invalid_tenant_empty() { - assertThatThrownByValidateKey("@a") - .hasMessage("Invalid input: empty tenant ID"); - assertThatThrownByValidateKey("a@") - .hasMessage("Invalid input: empty vendor"); - } - - @Test public void validateKey_invalid_tenant_vendor_longest() { - assertThatValidateKey("a@abcdef12345678").isTrue(); + @Test public void validateKey_shortest() { + for (char n = '0'; n <= '9'; n++) { + assertThatValidateKey(String.valueOf(n)).isTrue(); + } + for (char l = 'a'; l <= 'z'; l++) { + assertThatValidateKey(String.valueOf(l)).isTrue(); + } } - @Test public void validateKey_invalid_tenant_vendor_tooLong() { - assertThatThrownByValidateKey("a@abcdef1234567890") - .hasMessage("Invalid input: vendor too long"); + @Test public void validateKey_invalid_unicode() { + assertThatThrownByValidateKey("a💩") + .hasMessage("Invalid key: valid characters are: a-z 0-9 _ - * / @"); + assertThatThrownByValidateKey("💩a") + .hasMessage("Invalid key: must start with a-z 0-9"); } - static AbstractBooleanAssert assertThatValidateKey(String key) { - return assertThat(TracestateFormat.validateKey(key, true)); + AbstractBooleanAssert assertThatValidateKey(String key) { + return assertThat(tracestateFormat.validateKey(key, 0, key.length())); } - static AbstractThrowableAssert assertThatThrownByValidateKey(String key) { - return assertThatThrownBy(() -> TracestateFormat.validateKey(key, true)) + AbstractThrowableAssert assertThatThrownByValidateKey(String key) { + return assertThatThrownBy(() -> tracestateFormat.validateKey(key, 0, key.length())) .isInstanceOf(IllegalArgumentException.class); } }