From aa1265560b76c2c8009d47dd02b3c2447a660d8d Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 16 Mar 2020 20:41:52 +0800 Subject: [PATCH 01/13] Adds preconditions to extract to another PR --- brave/src/main/java/brave/propagation/TraceContext.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/brave/src/main/java/brave/propagation/TraceContext.java b/brave/src/main/java/brave/propagation/TraceContext.java index 2ea1214b82..1753b53a5a 100644 --- a/brave/src/main/java/brave/propagation/TraceContext.java +++ b/brave/src/main/java/brave/propagation/TraceContext.java @@ -518,7 +518,7 @@ public TraceContext build() { String missing = ""; if (traceIdHigh == 0L && traceId == 0L) missing += " traceId"; if (spanId == 0L) missing += " spanId"; - if (!"".equals(missing)) throw new IllegalArgumentException("Missing:" + missing); + if (!"".equals(missing)) throw new IllegalArgumentException("Missing :" + missing); return new TraceContext( flags, traceIdHigh, traceId, localRootId, parentId, spanId, ensureImmutable(extraList) ); From 3f3596b26788af86468d6e00f9067b810fc72e65 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 16 Mar 2020 20:57:16 +0800 Subject: [PATCH 02/13] WIP: tracecontext --- brave-bom/pom.xml | 5 + instrumentation/benchmarks/pom.xml | 6 + .../TraceContextPropagationBenchmarks.java | 108 +++++ pom.xml | 1 + propagation/pom.xml | 49 +++ propagation/w3c/RATIONALE.md | 62 +++ propagation/w3c/README.md | 14 + propagation/w3c/pom.xml | 64 +++ .../w3c/TraceContextExtractor.java | 83 ++++ .../propagation/w3c/TraceContextInjector.java | 52 +++ .../w3c/TraceContextPropagation.java | 95 +++++ .../propagation/w3c/TraceparentFormat.java | 297 ++++++++++++++ .../propagation/w3c/TracestateFormat.java | 124 ++++++ .../w3c/TraceContextPropagationTest.java | 110 +++++ .../w3c/TraceparentFormatTest.java | 383 ++++++++++++++++++ .../w3c/src/test/resources/log4j2.properties | 8 + 16 files changed, 1461 insertions(+) create mode 100644 instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java create mode 100644 propagation/pom.xml create mode 100644 propagation/w3c/RATIONALE.md create mode 100644 propagation/w3c/README.md create mode 100644 propagation/w3c/pom.xml create mode 100644 propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java create mode 100644 propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java create mode 100644 propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java create mode 100644 propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java create mode 100644 propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java create mode 100644 propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java create mode 100644 propagation/w3c/src/test/java/brave/propagation/w3c/TraceparentFormatTest.java create mode 100755 propagation/w3c/src/test/resources/log4j2.properties diff --git a/brave-bom/pom.xml b/brave-bom/pom.xml index 27149e3a30..193287b3e3 100644 --- a/brave-bom/pom.xml +++ b/brave-bom/pom.xml @@ -277,6 +277,11 @@ brave-spring-beans ${project.version} + + ${project.groupId} + brave-propagation-w3c + ${project.version} + diff --git a/instrumentation/benchmarks/pom.xml b/instrumentation/benchmarks/pom.xml index 6e0351d0f1..089008f2d1 100644 --- a/instrumentation/benchmarks/pom.xml +++ b/instrumentation/benchmarks/pom.xml @@ -239,6 +239,12 @@ grpc-testing ${grpc.version} + + + ${project.groupId} + brave-propagation-w3c + ${project.version} + diff --git a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java new file mode 100644 index 0000000000..b3b2e87a57 --- /dev/null +++ b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java @@ -0,0 +1,108 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import brave.internal.HexCodec; +import brave.propagation.Propagation; +import brave.propagation.TraceContext; +import brave.propagation.TraceContext.Extractor; +import brave.propagation.TraceContext.Injector; +import brave.propagation.TraceContextOrSamplingFlags; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +@Measurement(iterations = 5, time = 1) +@Warmup(iterations = 10, time = 1) +@Fork(3) +@BenchmarkMode(Mode.SampleTime) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +public class TraceContextPropagationBenchmarks { + static final Propagation tc = + TraceContextPropagation.FACTORY.create(Propagation.KeyFactory.STRING); + static final Injector> tcInjector = tc.injector(Map::put); + static final Extractor> tcExtractor = tc.extractor(Map::get); + + static final TraceContext context = TraceContext.newBuilder() + .traceIdHigh(HexCodec.lowerHexToUnsignedLong("67891233abcdef01")) + .traceId(HexCodec.lowerHexToUnsignedLong("2345678912345678")) + .spanId(HexCodec.lowerHexToUnsignedLong("463ac35c9f6413ad")) + .sampled(true) + .build(); + + // TODO: add tracestate examples which prefer the b3 entry + static final Map incoming128 = new LinkedHashMap() { + { + put("traceparent", TraceparentFormat.writeTraceparentFormat(context)); + } + }; + + static final Map incomingPadded = new LinkedHashMap() { + { + put("traceparent", + TraceparentFormat.writeTraceparentFormat(context.toBuilder().traceIdHigh(0).build())); + } + }; + + static final Map incomingMalformed = new LinkedHashMap() { + { + put("traceparent", "b970dafd-0d95-40aa-95d8-1d8725aebe40"); // not ok + } + }; + + static final Map nothingIncoming = Collections.emptyMap(); + + @Benchmark public void inject() { + Map carrier = new LinkedHashMap<>(); + tcInjector.inject(context, carrier); + } + + @Benchmark public TraceContextOrSamplingFlags extract_128() { + return tcExtractor.extract(incoming128); + } + + @Benchmark public TraceContextOrSamplingFlags extract_padded() { + return tcExtractor.extract(incomingPadded); + } + + @Benchmark public TraceContextOrSamplingFlags extract_nothing() { + return tcExtractor.extract(nothingIncoming); + } + + @Benchmark public TraceContextOrSamplingFlags extract_malformed() { + return tcExtractor.extract(incomingMalformed); + } + + // Convenience main entry-point + public static void main(String[] args) throws RunnerException { + Options opt = new OptionsBuilder() + .addProfiler("gc") + .include(".*" + TraceContextPropagationBenchmarks.class.getSimpleName()) + .build(); + + new Runner(opt).run(); + } +} diff --git a/pom.xml b/pom.xml index e9d58da0af..311fea4e75 100755 --- a/pom.xml +++ b/pom.xml @@ -136,6 +136,7 @@ brave-bom brave-tests context + propagation instrumentation spring-beans diff --git a/propagation/pom.xml b/propagation/pom.xml new file mode 100644 index 0000000000..e5582c3979 --- /dev/null +++ b/propagation/pom.xml @@ -0,0 +1,49 @@ + + + + 4.0.0 + + + io.zipkin.brave + brave-parent + 5.10.2-SNAPSHOT + + + brave-propagation-parent + Brave: Trace Propagation Formats + pom + + + ${project.basedir}/.. + + + + w3c + + + + + ${project.groupId} + brave + + + ${project.groupId} + brave-tests + test + + + diff --git a/propagation/w3c/RATIONALE.md b/propagation/w3c/RATIONALE.md new file mode 100644 index 0000000000..358d4267a4 --- /dev/null +++ b/propagation/w3c/RATIONALE.md @@ -0,0 +1,62 @@ +# brave-propagation-w3c rationale + +## Trace Context Specification + +### Why do we write a tracestate entry? +We write both "traceparent" and a "tracestate" entry for two reasons. The first is due to +incompatibility between the "traceparent" format and our trace context. This is described in another +section. + +The other reason is durability across hops. When your direct upstream is not the same tracing +system, its span ID (which they call parentId in section 3.2.2.4) will not be valid: using it will +break the trace. Instead, we look at our "tracestate" entry, which represents the last known place +in the same system. + +### What is supported by Brave, but not by traceparent format? + +#### `SamplingFlags` can precede a trace +B3 has always had the ability to influence the start of a trace with a sampling decision. For +example, you can propagate `b3=0` to force the next hop to not trace the request. This is used for +things like not sampling health checks. Conversely, you can force sampling by propagating `b3=d` +(the debug flag). `traceparent` requires a trace ID and a span ID, so cannot propagate this. + +#### `TraceIdContext` (trace ID only) +Amazon trace format can propagate a trace ID prior to starting a trace, for correlation purposes. +`traceparent` requires a trace ID and a span ID, so cannot a trace ID standalone. + +#### Not yet sampled/ deferred decision +B3 and Amazon formats support assigning a span ID prior to a sampling decision. `traceparent` has no +way to tell the difference between an explicit no and lack of a decision, as it only has one bit +flag. + +#### Debug flag +B3 has always had a debug flag, which is a way to force a trace even if normal sampling says no. +`traceparent` cannot distinguish between this and a normal decision, as it only has one bit flag. + +#### Trace-scoped sampling decision +`traceparent` does not distinguish between a hop-level or a trace scoped decision in the format. +This means that traces can be broken as it is valid to change the decision at every step (which +breaks the hierarchy). This is the main reason why we need a separate `tracestate` entry. + +### Why serialize the trace context in two formats? + +The "traceparent" header is only portable to get the `TraceContext.traceId()` and +`TraceContext.spanId()`. Section 3.2.2.5.1, the sampled flag, is incompatible with B3 sampling. The +format also lacks fields for `TraceContext.parentId()` and `TraceContext.debug()`. This requires us +to re-serialize the same context in two formats: one for compliance ("traceparent") and one that +actually stores the context (B3 single format). + +The impact on users will be higher overhead and confusion when comparing the sampled value of +"traceparent" which may be different than "b3". + +### Why is traceparent incompatible with B3? + +It may seem like incompatibility between "traceparent" and B3 were accidental, but that was not the +case. The Zipkin community held the first meetings leading to this specification, and were directly +involved in the initial design. Use cases of B3 were well known by working group members. Choices to +become incompatible with B3 (and Amazon X-Ray format) sampling were conscious, as were decisions to +omit other fields we use. These decisions were made in spite of protest from Zipkin community +members and others. There is a rationale document for the specification, but the working group has +so far not explained these decisions, or even mention B3 at all. + +https://github.com/w3c/trace-context/blob/d2ed8084780efcedab7e60c48b06ca3c00bea55c/http_header_format_rationale.md diff --git a/propagation/w3c/README.md b/propagation/w3c/README.md new file mode 100644 index 0000000000..ca53626ded --- /dev/null +++ b/propagation/w3c/README.md @@ -0,0 +1,14 @@ +# brave-propagation-w3c + +This project includes propagation handlers for W3C defined headers. + +## Trace Context +The [Trace Context][https://w3c.github.io/trace-context/] specification defines two headers: + + * `traceparent` - almost the same as our [B3-single format](https://github.com/openzipkin/b3-propagation#single-header) + * `tracestate` - vendor-specific (or format-specific): may impact how to interpret `traceparent` + +This implementation can survive mixed systems who follow the specification and forward the +`tracestate` header. When writing the `traceparent` header, this also overwrites the `tracestate` +entry named 'b3' (in B3 single format). When reading headers, this entry is favored over the +`traceparent`, allowing the the next span to re-attach to the last known 'b3' header. diff --git a/propagation/w3c/pom.xml b/propagation/w3c/pom.xml new file mode 100644 index 0000000000..4f975e78dd --- /dev/null +++ b/propagation/w3c/pom.xml @@ -0,0 +1,64 @@ + + + + + io.zipkin.brave + brave-propagation-parent + 5.10.2-SNAPSHOT + + 4.0.0 + + brave-propagation-w3c + Brave Propagation: W3C Tracing headers (traceparent, tracestate, etc.) + + + ${project.basedir}/../.. + 1.6 + java16 + + + + + + org.powermock + powermock-module-junit4 + ${powermock.version} + test + + + org.powermock + powermock-api-mockito2 + ${powermock.version} + test + + + + + + + maven-jar-plugin + + + + brave.propagation.w3c + + + + + + + diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java new file mode 100644 index 0000000000..6a4abc2191 --- /dev/null +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java @@ -0,0 +1,83 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import brave.propagation.Propagation.Getter; +import brave.propagation.SamplingFlags; +import brave.propagation.TraceContext; +import brave.propagation.TraceContext.Extractor; +import brave.propagation.TraceContextOrSamplingFlags; +import brave.propagation.w3c.TraceContextPropagation.Extra; +import java.util.Collections; +import java.util.List; + +import static brave.propagation.B3SingleFormat.parseB3SingleFormat; + +final class TraceContextExtractor implements Extractor { + final Getter getter; + final K tracestateKey; + final TracestateFormat tracestateFormat; + + TraceContextExtractor(TraceContextPropagation propagation, Getter getter) { + this.getter = getter; + this.tracestateKey = propagation.tracestateKey; + this.tracestateFormat = new TracestateFormat(propagation.stateName); + } + + @Override public TraceContextOrSamplingFlags extract(C carrier) { + if (carrier == null) throw new NullPointerException("carrier == null"); + String value = getter.get(carrier, tracestateKey); + if (value == null) return EMPTY; + + B3SingleFormatHandler handler = new B3SingleFormatHandler(); + CharSequence otherEntries = tracestateFormat.parseAndReturnOtherEntries(value, handler); + + List extra; + if (otherEntries == null) { + extra = DEFAULT_EXTRA; + } else { + Extra e = new Extra(); + e.otherEntries = otherEntries; + extra = Collections.singletonList(e); + } + + TraceContext context = handler.context; + if (context == null) { + if (extra == DEFAULT_EXTRA) return EMPTY; + return TraceContextOrSamplingFlags.newBuilder() + .extra(extra) + .samplingFlags(SamplingFlags.EMPTY) + .build(); + } + return TraceContextOrSamplingFlags.newBuilder().context(context).extra(extra).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; + } + } + + /** When present, this context was created with TracestatePropagation */ + static final Extra MARKER = new Extra(); + + static final List DEFAULT_EXTRA = Collections.singletonList(MARKER); + static final TraceContextOrSamplingFlags EMPTY = + TraceContextOrSamplingFlags.EMPTY.toBuilder().extra(DEFAULT_EXTRA).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 new file mode 100644 index 0000000000..b0b6dc4d17 --- /dev/null +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java @@ -0,0 +1,52 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import brave.propagation.Propagation.Setter; +import brave.propagation.TraceContext; +import brave.propagation.TraceContext.Injector; +import brave.propagation.w3c.TraceContextPropagation.Extra; + +import static brave.propagation.B3SingleFormat.writeB3SingleFormat; +import static brave.propagation.w3c.TraceparentFormat.writeTraceparentFormat; + +final class TraceContextInjector implements Injector { + final TracestateFormat tracestateFormat; + final Setter setter; + final K traceparentKey, tracestateKey; + + TraceContextInjector(TraceContextPropagation propagation, Setter setter) { + this.tracestateFormat = new TracestateFormat(propagation.stateName); + this.traceparentKey = propagation.traceparentKey; + this.tracestateKey = propagation.tracestateKey; + this.setter = setter; + } + + @Override public void inject(TraceContext traceContext, C carrier) { + + setter.put(carrier, 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 Extra) { + otherState = ((Extra) next).otherEntries; + break; + } + } + + String tracestate = tracestateFormat.write(writeB3SingleFormat(traceContext), otherState); + setter.put(carrier, tracestateKey, tracestate); + } +} diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java new file mode 100644 index 0000000000..f539af5c78 --- /dev/null +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java @@ -0,0 +1,95 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +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.List; + +public final class TraceContextPropagation implements Propagation { + // TODO: not sure if we will want a constant here, or something like a builder. For example, we + // probably will need a primary state handler (ex b3) to catch data no longer in the traceparent + // format. At any rate, we will need to know what state is primarily ours, so that probably means + // not having a constant, unless that constant uses b3 single impl for the tracestate entry. + public static final Propagation.Factory FACTORY = + new Propagation.Factory() { + @Override public Propagation create(KeyFactory keyFactory) { + return new TraceContextPropagation<>(keyFactory); + } + + /** + * Traceparent doesn't support sharing the same span ID, though it may be possible to support + * this by extending it with a b3 state entry. + */ + @Override public boolean supportsJoin() { + return false; + } + + @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; + } + + @Override public String toString() { + return "TracestatePropagationFactory"; + } + }; + + final String stateName; + final K traceparentKey, tracestateKey; + final List fields; + + TraceContextPropagation(KeyFactory keyFactory) { + this.stateName = "b3"; + this.traceparentKey = keyFactory.create("traceparent"); + this.tracestateKey = keyFactory.create("tracestate"); + this.fields = Arrays.asList(traceparentKey, tracestateKey); + } + + @Override public List keys() { + return fields; + } + + @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) { + if (getter == null) throw new NullPointerException("getter == null"); + return new TraceContextExtractor<>(this, getter); + } + + /** + * This only contains other entries. The entry for the current trace is only written during + * injection. + */ + static final class Extra { // hidden intentionally + CharSequence otherEntries; + + @Override public String toString() { + return "TracestatePropagation{" + + (otherEntries != null ? ("entries=" + otherEntries.toString()) : "") + + "}"; + } + } +} diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java new file mode 100644 index 0000000000..9f17ade44e --- /dev/null +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java @@ -0,0 +1,297 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import brave.internal.Nullable; +import brave.internal.Platform; +import brave.propagation.TraceContext; +import java.lang.ref.WeakReference; +import java.nio.ByteBuffer; + +import static brave.internal.HexCodec.writeHexLong; + +/** Implements https://w3c.github.io/trace-context/#traceparent-header */ +final class TraceparentFormat { + /** Version '00' is fixed length, though future versions may be longer. */ + static final int FORMAT_LENGTH = 3 + 32 + 1 + 16 + 3; // 00-traceid128-spanid-01 + + static final int // instead of enum for smaller bytecode + FIELD_VERSION = 1, + FIELD_TRACE_ID = 2, + FIELD_PARENT_ID = 3, + FIELD_TRACE_FLAGS = 4; + + /** Writes all "traceparent" defined fields in the trace context to a hyphen delimited string. */ + public static String writeTraceparentFormat(TraceContext context) { + char[] buffer = getCharBuffer(); + int length = writeTraceparentFormat(context, buffer); + return new String(buffer, 0, length); + } + + /** + * Like {@link #writeTraceparentFormat(TraceContext)}, but for carriers with byte array or byte + * buffer values. For example, {@link ByteBuffer#wrap(byte[])} can wrap the result. + */ + public static byte[] writeTraceparentFormatAsBytes(TraceContext context) { + char[] buffer = getCharBuffer(); + int length = writeTraceparentFormat(context, buffer); + return asciiToNewByteArray(buffer, length); + } + + static int writeTraceparentFormat(TraceContext context, char[] result) { + int pos = 0; + result[pos++] = '0'; + result[pos++] = '0'; + result[pos++] = '-'; + long traceIdHigh = context.traceIdHigh(); + writeHexLong(result, pos, traceIdHigh); + pos += 16; + writeHexLong(result, pos, context.traceId()); + pos += 16; + result[pos++] = '-'; + writeHexLong(result, pos, context.spanId()); + pos += 16; + + result[pos++] = '-'; + result[pos++] = '0'; + result[pos++] = Boolean.TRUE.equals(context.sampled()) ? '1' : '0'; + + return pos; + } + + @Nullable + public static TraceContext parseTraceparentFormat(CharSequence parent) { + return parseTraceparentFormat(parent, 0, parent.length()); + } + + /** + * This reads a trace context a sequence potentially larger than the format. The use-case is + * reducing garbage, by re-using the input {@code value} across multiple parse operations. + * + * @param value the sequence that contains a {@code traceparent} formatted trace context + * @param beginIndex the inclusive begin index: {@linkplain CharSequence#charAt(int) index} of the + * first character in {@code traceparent} format. + * @param endIndex the exclusive end index: {@linkplain CharSequence#charAt(int) index} + * after the last character in {@code traceparent} format. + */ + @Nullable + public static TraceContext parseTraceparentFormat(CharSequence value, int beginIndex, + int endIndex) { + int length = endIndex - beginIndex; + + if (length == 0) { + Platform.get().log("Invalid input: empty", null); + return null; + } + + int version = 0; + TraceContext.Builder builder = getBuilder(); + boolean traceIdHighZero = false; + + int currentField = FIELD_VERSION, currentFieldLength = 0; + // Used for hex-decoding, performed by bitwise addition + long buffer = 0L; + + // Instead of pos < endIndex, this uses pos <= endIndex to keep field processing consolidated. + // Otherwise, we'd have to process again when outside the loop to handle dangling data on EOF. + LOOP: + for (int pos = beginIndex; pos <= endIndex; pos++) { + // treat EOF same as a hyphen for simplicity + boolean isEof = pos == endIndex; + char c = isEof ? '-' : value.charAt(pos); + + if (c == '-') { + if (!validateFieldLength(currentField, currentFieldLength)) { + return null; + } + + switch (currentField) { + case FIELD_VERSION: + // 8-bit unsigned 255 is disallowed https://w3c.github.io/trace-context/#version + version = (int) buffer; + if (version == 0xff) { + log(currentField, "Invalid input: ff {0}"); + return null; + } else if (version == 0 && length > FORMAT_LENGTH) { + Platform.get().log("Invalid input: too long", null); + return null; + } + + currentField = FIELD_TRACE_ID; + break; + case FIELD_TRACE_ID: + if (traceIdHighZero && buffer == 0L) { + logReadAllZeros(currentField); + return null; + } + + builder.traceId(buffer); + + currentField = FIELD_PARENT_ID; + break; + case FIELD_PARENT_ID: + if (buffer == 0L) { + logReadAllZeros(currentField); + return null; + } + + builder.spanId(buffer); + + currentField = FIELD_TRACE_FLAGS; + break; + case FIELD_TRACE_FLAGS: + int traceparentFlags = (int) (buffer & 0xff); + // Only one flag is defined at version 0: sampled + // https://w3c.github.io/trace-context/#sampled-flag + builder.sampled(((traceparentFlags & 1) == 1)); + + // If the version is greater than zero ignore other flags and fields + // https://w3c.github.io/trace-context/#other-flags + if (version == 0) { + if ((traceparentFlags & ~1) != 0) { + log(currentField, "Invalid input: only choices are 00 or 01 {0}"); + return null; + } + + if (!isEof) { + Platform.get().log("Invalid input: more than 3 fields exist", null); + return null; + } + } + break LOOP; + default: + throw new AssertionError(); + } + + buffer = 0L; + currentFieldLength = 0; + continue; + } + + // At this point, 'c' is not a hyphen + + // When we get to a non-hyphen at position 16, we have a 128-bit trace ID. + if (currentField == FIELD_TRACE_ID && currentFieldLength == 16) { + // traceIdHigh can be zeros when a 64-bit trace ID is encoded in 128-bits. + traceIdHighZero = buffer == 0L; + builder.traceIdHigh(buffer); + + // This character is the next hex. If it isn't, the next iteration will throw. Either way, + // reset so that we can capture the next 16 characters of the trace ID. + buffer = 0L; + } + + currentFieldLength++; + + // The rest of this is normal lower-hex decoding + buffer <<= 4; + if (c >= '0' && c <= '9') { + buffer |= c - '0'; + } else if (c >= 'a' && c <= 'f') { + buffer |= c - 'a' + 10; + } else { + log(currentField, "Invalid input: only valid characters are lower-hex for {0}"); + return null; + } + } + + try { + return builder.build(); + } catch (IllegalArgumentException e) { // trace ID or span ID were all zeros + Platform.get().log(e.getMessage(), null); + return null; + } + } + + static boolean validateFieldLength(int field, int length) { + int expectedLength = + (field == FIELD_TRACE_FLAGS || field == FIELD_VERSION) ? 2 + : field == FIELD_TRACE_ID ? 32 : 16; + if (length == 0) { + log(field, "Invalid input: empty {0}"); + return false; + } else if (length < expectedLength) { + log(field, "Invalid input: {0} is too short"); + return false; + } else if (length > expectedLength) { + log(field, "Invalid input: {0} is too long"); + return false; + } + return true; + } + + static void logReadAllZeros(int currentField) { + log(currentField, "Invalid input: read all zeros {0}"); + } + + static void log(int fieldCode, String s) { + String field; + switch (fieldCode) { + case FIELD_VERSION: + field = "version"; + break; + case FIELD_TRACE_ID: + field = "trace ID"; + break; + // Confusingly, the spec calls the span ID field parentId + // https://w3c.github.io/trace-context/#parent-id + case FIELD_PARENT_ID: + field = "parent ID"; + break; + case FIELD_TRACE_FLAGS: + field = "trace flags"; + break; + default: + throw new AssertionError("field code unmatched: " + fieldCode); + } + Platform.get().log(s, field, null); + } + + static byte[] asciiToNewByteArray(char[] buffer, int length) { + byte[] result = new byte[length]; + for (int i = 0; i < length; i++) { + result[i] = (byte) buffer[i]; + } + return result; + } + + static final ThreadLocal> BUILDER_REF = new ThreadLocal<>(); + + static TraceContext.Builder getBuilder() { + WeakReference ref = BUILDER_REF.get(); + TraceContext.Builder builder; + if (ref == null || (builder = ref.get()) == null) { + builder = TraceContext.newBuilder(); + ref = new WeakReference<>(builder); + BUILDER_REF.set(ref); + } else { + builder.clear(); + } + return builder; + } + + static final ThreadLocal CHAR_BUFFER = new ThreadLocal<>(); + + static char[] getCharBuffer() { + char[] charBuffer = CHAR_BUFFER.get(); + if (charBuffer == null) { + charBuffer = new char[FORMAT_LENGTH]; + CHAR_BUFFER.set(charBuffer); + } + return charBuffer; + } + + TraceparentFormat() { + } +} diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java new file mode 100644 index 0000000000..e8ab6f222e --- /dev/null +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java @@ -0,0 +1,124 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import brave.internal.Nullable; + +import static brave.propagation.w3c.TraceparentFormat.FORMAT_LENGTH; + +/** + * Implements https://w3c.github.io/trace-context/#tracestate-header + * + *

In the above specification, a tracestate entry is sometimes called member. The key of the + * entry is most often called vendor name, but it is more about a tracing system vs something vendor + * 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); + } + + // 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= ' + @Nullable CharSequence 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; + } + } + return otherEntries; + } +} diff --git a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java new file mode 100644 index 0000000000..0276171b56 --- /dev/null +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java @@ -0,0 +1,110 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import brave.propagation.Propagation.KeyFactory; +import brave.propagation.TraceContext; +import brave.propagation.TraceContext.Extractor; +import brave.propagation.TraceContext.Injector; +import brave.propagation.TraceContextOrSamplingFlags; +import brave.propagation.w3c.TraceContextPropagation.Extra; +import java.util.LinkedHashMap; +import java.util.Map; +import org.junit.Test; + +import static brave.internal.HexCodec.lowerHexToUnsignedLong; +import static java.util.Arrays.asList; +import static org.assertj.core.api.Assertions.assertThat; + +public class TraceContextPropagationTest { + Map carrier = new LinkedHashMap<>(); + Injector> injector = + TraceContextPropagation.FACTORY.create(KeyFactory.STRING).injector(Map::put); + Extractor> extractor = + TraceContextPropagation.FACTORY.create(KeyFactory.STRING).extractor(Map::get); + + TraceContext sampledContext = + TraceContext.newBuilder() + .traceIdHigh(lowerHexToUnsignedLong("67891233abcdef01")) + .traceId(lowerHexToUnsignedLong("2345678912345678")) + .spanId(lowerHexToUnsignedLong("463ac35c9f6413ad")) + .sampled(true) + .build(); + String validTraceparent = "00-67891233abcdef012345678912345678-463ac35c9f6413ad-01"; + String validB3Single = "67891233abcdef012345678912345678-463ac35c9f6413ad-1"; + String otherState = "congo=lZWRzIHRoNhcm5hbCBwbGVhc3VyZS4="; + + @Test public void injects_b3_when_no_other_tracestate() { + Extra extra = new Extra(); + + sampledContext = sampledContext.toBuilder().extra(asList(extra)).build(); + + injector.inject(sampledContext, carrier); + + assertThat(carrier).containsEntry("tracestate", "b3=" + validB3Single); + } + + @Test public void injects_b3_before_other_tracestate() { + Extra extra = new Extra(); + extra.otherEntries = otherState; + + sampledContext = sampledContext.toBuilder().extra(asList(extra)).build(); + + injector.inject(sampledContext, carrier); + + assertThat(carrier).containsEntry("tracestate", "b3=" + validB3Single + "," + otherState); + } + + @Test public void extracts_b3_when_no_other_tracestate() { + carrier.put("traceparent", validTraceparent); + carrier.put("tracestate", "b3=" + validB3Single); + + assertThat(extractor.extract(carrier)) + .isEqualTo( + TraceContextOrSamplingFlags.newBuilder() + .addExtra(new Extra()) + .context(sampledContext) + .build()); + } + + @Test public void extracts_b3_before_other_tracestate() { + carrier.put("traceparent", validTraceparent); + carrier.put("tracestate", "b3=" + validB3Single + "," + otherState); + + Extra extra = new Extra(); + extra.otherEntries = otherState; + + assertThat(extractor.extract(carrier)) + .isEqualTo( + TraceContextOrSamplingFlags.newBuilder() + .addExtra(extra) + .context(sampledContext) + .build()); + } + + @Test public void extracts_b3_after_other_tracestate() { + carrier.put("traceparent", validTraceparent); + carrier.put("tracestate", otherState + ",b3=" + validB3Single); + + Extra extra = new Extra(); + extra.otherEntries = otherState; + + assertThat(extractor.extract(carrier)) + .isEqualTo( + TraceContextOrSamplingFlags.newBuilder() + .addExtra(extra) + .context(sampledContext) + .build()); + } +} diff --git a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceparentFormatTest.java b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceparentFormatTest.java new file mode 100644 index 0000000000..8870911c77 --- /dev/null +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceparentFormatTest.java @@ -0,0 +1,383 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import brave.internal.Platform; +import brave.propagation.TraceContext; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.powermock.core.classloader.annotations.PowerMockIgnore; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +import static brave.propagation.w3c.TraceparentFormat.parseTraceparentFormat; +import static brave.propagation.w3c.TraceparentFormat.writeTraceparentFormat; +import static brave.propagation.w3c.TraceparentFormat.writeTraceparentFormatAsBytes; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.powermock.api.mockito.PowerMockito.mock; +import static org.powermock.api.mockito.PowerMockito.mockStatic; +import static org.powermock.api.mockito.PowerMockito.when; + +@RunWith(PowerMockRunner.class) +// Added to declutter console: tells power mock not to mess with implicit classes we aren't testing +@PowerMockIgnore({"org.apache.logging.*", "javax.script.*"}) +@PrepareForTest({Platform.class, TraceparentFormat.class}) +public class TraceparentFormatTest { + String traceIdHigh = "1234567890123459"; + String traceId = "1234567890123451"; + String parentId = "1234567890123452"; + String spanId = "1234567890123453"; + + Platform platform = mock(Platform.class); + + @Before public void setupLogger() { + mockStatic(Platform.class); + when(Platform.get()).thenReturn(platform); + } + + /** Either we asserted on the log messages or there weren't any */ + @After public void ensureNothingLogged() { + verifyNoMoreInteractions(platform); + } + + /** unsampled isn't the same as not-yet-sampled, but we have no better choice */ + @Test public void writeTraceparentFormat_notYetSampled_128() { + TraceContext context = TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)).build(); + + assertThat(writeTraceparentFormat(context)) + .isEqualTo("00-" + traceIdHigh + traceId + "-" + spanId + "-00") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + } + + @Test public void writeTraceparentFormat_unsampled() { + TraceContext context = TraceContext.newBuilder() + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(false).build(); + + assertThat(writeTraceparentFormat(context)) + .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-00") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + } + + @Test public void writeTraceparentFormat_sampled() { + TraceContext context = TraceContext.newBuilder() + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build(); + + assertThat(writeTraceparentFormat(context)) + .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-01") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + } + + /** debug isn't the same as sampled, but we have no better choice */ + @Test public void writeTraceparentFormat_debug() { + TraceContext context = TraceContext.newBuilder() + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .debug(true).build(); + + assertThat(writeTraceparentFormat(context)) + .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-01") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + } + + /** + * There is no field for the parent ID in "traceparent" format. What it calls "parent ID" is + * actually the span ID. + */ + @Test public void writeTraceparentFormat_parent() { + TraceContext context = TraceContext.newBuilder() + .traceId(Long.parseUnsignedLong(traceId, 16)) + .parentId(Long.parseUnsignedLong(parentId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build(); + + assertThat(writeTraceparentFormat(context)) + .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-01") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + } + + @Test public void writeTraceparentFormat_largest() { + TraceContext context = TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .parentId(Long.parseUnsignedLong(parentId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .debug(true).build(); + + assertThat(writeTraceparentFormat(context)) + .isEqualTo("00-" + traceIdHigh + traceId + "-" + spanId + "-01") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + } + + @Test public void parseTraceparentFormat_sampled() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-01")) + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); + } + + @Test public void parseTraceparentFormat_unsampled() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-00")) + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(false).build() + ); + } + + @Test public void parseTraceparentFormat_padded() { + assertThat(parseTraceparentFormat("00-0000000000000000" + traceId + "-" + spanId + "-01")) + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); + } + + @Test public void parseTraceparentFormat_padded_right() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + "0000000000000000-" + spanId + "-01")) + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); + } + + @Test public void parseTraceparentFormat_newer_version() { + assertThat(parseTraceparentFormat("10-" + traceIdHigh + traceId + "-" + spanId + "-00")) + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(false).build() + ); + } + + @Test public void parseTraceparentFormat_newer_version_ignores_extra_fields() { + assertThat(parseTraceparentFormat("10-" + traceIdHigh + traceId + "-" + spanId + "-00-fobaly")) + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(false).build() + ); + } + + @Test public void parseTraceparentFormat_newer_version_ignores_extra_flags() { + assertThat(parseTraceparentFormat("10-" + traceIdHigh + traceId + "-" + spanId + "-ff")) + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); + } + + /** for example, parsing inside tracestate */ + @Test public void parseTraceparentFormat_middleOfString() { + String input = "tc=00-" + traceIdHigh + traceId + "-" + spanId + "-01,"; + assertThat(parseTraceparentFormat(input, 3, input.length() - 1)) + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); + } + + @Test public void parseTraceparentFormat_middleOfString_incorrectIndex() { + String input = "tc=00-" + traceIdHigh + traceId + "-" + spanId + "-00,"; + assertThat(parseTraceparentFormat(input, 0, 12)) + .isNull(); // instead of raising exception + + verify(platform) + .log("Invalid input: only valid characters are lower-hex for {0}", "version", null); + } + + /** This tests that the being index is inclusive and the end index is exclusive */ + @Test public void parseTraceparentFormat_ignoresBeforeAndAfter() { + String encoded = "00-" + traceIdHigh + traceId + "-" + spanId + "-01"; + String sequence = "??" + encoded + "??"; + assertThat(parseTraceparentFormat(sequence, 2, 2 + encoded.length())) + .isEqualToComparingFieldByField(parseTraceparentFormat(encoded)); + } + + @Test public void parseTraceparentFormat_malformed() { + assertThat(parseTraceparentFormat("not-a-tumor")) + .isNull(); // instead of raising exception + + verify(platform) + .log("Invalid input: only valid characters are lower-hex for {0}", "version", null); + } + + @Test public void parseTraceparentFormat_malformed_notAscii() { + assertThat(parseTraceparentFormat( + "00-" + traceIdHigh + traceId + "-" + spanId.substring(0, 15) + "💩-1")) + .isNull(); // instead of crashing + + verify(platform) + .log("Invalid input: only valid characters are lower-hex for {0}", "parent ID", null); + } + + @Test public void parseTraceparentFormat_malformed_uuid() { + assertThat(parseTraceparentFormat("b970dafd-0d95-40aa-95d8-1d8725aebe40")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: {0} is too long", "version", null); + } + + @Test public void parseTraceparentFormat_short_traceId() { + assertThat( + parseTraceparentFormat("00-" + traceId + "-" + spanId + "-01")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: {0} is too short", "trace ID", null); + } + + @Test public void parseTraceparentFormat_zero_traceId() { + assertThat( + parseTraceparentFormat("00-00000000000000000000000000000000-" + spanId + "-01")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: read all zeros {0}", "trace ID", null); + } + + @Test public void parseTraceparentFormat_fails_on_extra_flags() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-ff")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: only choices are 00 or 01 {0}", "trace flags", null); + } + + @Test public void parseTraceparentFormat_fails_on_extra_fields() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-0-")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: {0} is too short", "trace flags", null); + } + + @Test public void parseTraceparentFormat_fails_on_version_ff() { + assertThat(parseTraceparentFormat("ff-" + traceIdHigh + traceId + "-" + spanId + "-01")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: ff {0}", "version", null); + } + + @Test public void parseTraceparentFormat_zero_spanId() { + assertThat( + parseTraceparentFormat("00-" + traceIdHigh + traceId + "-0000000000000000-01")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: read all zeros {0}", "parent ID", null); + } + + @Test public void parseTraceparentFormat_empty() { + assertThat(parseTraceparentFormat("")).isNull(); + + verify(platform).log("Invalid input: empty", null); + } + + @Test public void parseTraceparentFormat_empty_version() { + assertThat(parseTraceparentFormat("-" + traceIdHigh + traceId + "-" + spanId + "-00")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: empty {0}", "version", null); + } + + @Test public void parseTraceparentFormat_empty_traceId() { + assertThat(parseTraceparentFormat("00--" + spanId + "-00")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: empty {0}", "trace ID", null); + } + + @Test public void parseTraceparentFormat_empty_spanId() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "--01")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: empty {0}", "parent ID", null); + } + + @Test public void parseTraceparentFormat_empty_flags() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: empty {0}", "trace flags", null); + } + + @Test public void parseTraceparentFormat_truncated_traceId() { + assertThat(parseTraceparentFormat("00-1-" + spanId + "-01")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: {0} is too short", "trace ID", null); + } + + @Test public void parseTraceparentFormat_truncated_traceId128() { + assertThat(parseTraceparentFormat("00-1" + traceId + "-" + spanId + "-01")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: {0} is too short", "trace ID", null); + } + + @Test public void parseTraceparentFormat_truncated_spanId() { + assertThat( + parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId.substring(0, 15) + "-00")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: {0} is too short", "parent ID", null); + } + + @Test public void parseTraceparentFormat_truncated_flags() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-0")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: {0} is too short", "trace flags", null); + } + + @Test public void parseTraceparentFormat_traceIdTooLong() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "a" + "-" + spanId + "-0")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: {0} is too long", "trace ID", null); + } + + @Test public void parseTraceparentFormat_spanIdTooLong() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "a-0")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: {0} is too long", "parent ID", null); + } + + @Test public void parseTraceparentFormat_flagsTooLong() { + assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-001")) + .isNull(); // instead of raising exception + + verify(platform).log("Invalid input: too long", null); + } +} diff --git a/propagation/w3c/src/test/resources/log4j2.properties b/propagation/w3c/src/test/resources/log4j2.properties new file mode 100755 index 0000000000..5d75397687 --- /dev/null +++ b/propagation/w3c/src/test/resources/log4j2.properties @@ -0,0 +1,8 @@ +appenders=console +appender.console.type=Console +appender.console.name=STDOUT +appender.console.layout.type=PatternLayout +appender.console.layout.pattern=%d{ABSOLUTE} %-5p [%t] %C{2} (%F:%L) [%X{traceId}/%X{spanId}] - %m%n +rootLogger.level=debug +rootLogger.appenderRefs=stdout +rootLogger.appenderRef.stdout.ref=STDOUT From 6b253e5d946aa56839de3ac7fb35f780da8d0465 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 16 Mar 2020 21:11:03 +0800 Subject: [PATCH 03/13] notes and dead code removal --- .../brave/propagation/w3c/TraceparentFormat.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 9f17ade44e..c8832dd307 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java @@ -22,6 +22,8 @@ import static brave.internal.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 +// expensive. We should inline that here to to unhook the internal dep. final class TraceparentFormat { /** Version '00' is fixed length, though future versions may be longer. */ static final int FORMAT_LENGTH = 3 + 32 + 1 + 16 + 3; // 00-traceid128-spanid-01 @@ -206,12 +208,7 @@ public static TraceContext parseTraceparentFormat(CharSequence value, int beginI } } - try { - return builder.build(); - } catch (IllegalArgumentException e) { // trace ID or span ID were all zeros - Platform.get().log(e.getMessage(), null); - return null; - } + return builder.build(); } static boolean validateFieldLength(int field, int length) { @@ -266,8 +263,11 @@ static byte[] asciiToNewByteArray(char[] buffer, int length) { return result; } + // WeakReference to ensure our classes can be unloaded. + // TODO: add classloader test static final ThreadLocal> BUILDER_REF = new ThreadLocal<>(); + // TODO: check with benchmark that this is notably better than just newing up each time. static TraceContext.Builder getBuilder() { WeakReference ref = BUILDER_REF.get(); TraceContext.Builder builder; From c6ef5097ff3240c8b286ff6fd0983ad9ae2f4a4e Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Tue, 17 Mar 2020 10:22:46 +0800 Subject: [PATCH 04/13] reverts fancy threadlocal which actually hurts more than helps --- .../TraceContextPropagationBenchmarks.java | 6 ++-- .../w3c/TraceContextExtractor.java | 30 +++++++++++++++---- .../propagation/w3c/TraceparentFormat.java | 22 ++------------ 3 files changed, 31 insertions(+), 27 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 b3b2e87a57..a119ede808 100644 --- a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java @@ -54,7 +54,7 @@ public class TraceContextPropagationBenchmarks { .build(); // TODO: add tracestate examples which prefer the b3 entry - static final Map incoming128 = new LinkedHashMap() { + static final Map incoming = new LinkedHashMap() { { put("traceparent", TraceparentFormat.writeTraceparentFormat(context)); } @@ -80,8 +80,8 @@ public class TraceContextPropagationBenchmarks { tcInjector.inject(context, carrier); } - @Benchmark public TraceContextOrSamplingFlags extract_128() { - return tcExtractor.extract(incoming128); + @Benchmark public TraceContextOrSamplingFlags extract() { + return tcExtractor.extract(incoming); } @Benchmark public TraceContextOrSamplingFlags extract_padded() { 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 6a4abc2191..f5d1ed4497 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java @@ -24,24 +24,44 @@ import static brave.propagation.B3SingleFormat.parseB3SingleFormat; +// TODO: this class has no useful tests wrt traceparent yet final class TraceContextExtractor implements Extractor { final Getter getter; - final K tracestateKey; + final K traceparentKey, tracestateKey; final TracestateFormat tracestateFormat; + final B3SingleFormatHandler handler = new B3SingleFormatHandler(); TraceContextExtractor(TraceContextPropagation propagation, Getter getter) { this.getter = getter; + this.traceparentKey = propagation.traceparentKey; this.tracestateKey = propagation.tracestateKey; this.tracestateFormat = new TracestateFormat(propagation.stateName); } @Override public TraceContextOrSamplingFlags extract(C carrier) { if (carrier == null) throw new NullPointerException("carrier == null"); - String value = getter.get(carrier, tracestateKey); - if (value == null) return EMPTY; + String traceparent = getter.get(carrier, traceparentKey); + if (traceparent == null) return EMPTY; - B3SingleFormatHandler handler = new B3SingleFormatHandler(); - CharSequence otherEntries = tracestateFormat.parseAndReturnOtherEntries(value, handler); + // TODO: add link that says tracestate itself is optional + String tracestate = getter.get(carrier, tracestateKey); + if (tracestate == 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 + // never be traced! + // + // NOTE: We are required to use the same trace ID, there's some vagueness about the parent + // span ID. Ex we don't know if upstream are sending to the same system or not, when we can't + // 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(traceparent); + return TraceContextOrSamplingFlags.newBuilder() + .context(maybeUpstream) + .extra(DEFAULT_EXTRA) // marker for outbound propagation + .build(); + } + + CharSequence otherEntries = tracestateFormat.parseAndReturnOtherEntries(tracestate, handler); List extra; if (otherEntries == null) { 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 c8832dd307..7060c88bea 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java @@ -97,8 +97,10 @@ public static TraceContext parseTraceparentFormat(CharSequence value, int beginI return null; } + // Benchmarks show no difference in memory usage re-using with thread local vs newing each time. + TraceContext.Builder builder = TraceContext.newBuilder(); + int version = 0; - TraceContext.Builder builder = getBuilder(); boolean traceIdHighZero = false; int currentField = FIELD_VERSION, currentFieldLength = 0; @@ -263,24 +265,6 @@ static byte[] asciiToNewByteArray(char[] buffer, int length) { return result; } - // WeakReference to ensure our classes can be unloaded. - // TODO: add classloader test - static final ThreadLocal> BUILDER_REF = new ThreadLocal<>(); - - // TODO: check with benchmark that this is notably better than just newing up each time. - static TraceContext.Builder getBuilder() { - WeakReference ref = BUILDER_REF.get(); - TraceContext.Builder builder; - if (ref == null || (builder = ref.get()) == null) { - builder = TraceContext.newBuilder(); - ref = new WeakReference<>(builder); - BUILDER_REF.set(ref); - } else { - builder.clear(); - } - return builder; - } - static final ThreadLocal CHAR_BUFFER = new ThreadLocal<>(); static char[] getCharBuffer() { From 0962de6dc3a9caab10e8ed544ea8ef95507a9fb0 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Tue, 17 Mar 2020 11:39:01 +0800 Subject: [PATCH 05/13] adds classloader test --- propagation/w3c/RATIONALE.md | 8 ++- ...raceContextPropagationClassLoaderTest.java | 53 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java diff --git a/propagation/w3c/RATIONALE.md b/propagation/w3c/RATIONALE.md index 358d4267a4..e16b5ab9d5 100644 --- a/propagation/w3c/RATIONALE.md +++ b/propagation/w3c/RATIONALE.md @@ -24,11 +24,17 @@ things like not sampling health checks. Conversely, you can force sampling by pr Amazon trace format can propagate a trace ID prior to starting a trace, for correlation purposes. `traceparent` requires a trace ID and a span ID, so cannot a trace ID standalone. -#### Not yet sampled/ deferred decision +#### `TraceContext.sampled() == null` B3 and Amazon formats support assigning a span ID prior to a sampling decision. `traceparent` has no way to tell the difference between an explicit no and lack of a decision, as it only has one bit flag. +#### `TraceContext.parentId()` +The parentId is a propagated field and also used in logging expressions. This is important for RPC +spans as downstream usually finishes before upstream. This obviates a data race even if Zipkin's UI +can tolerate lack of parent ID. What `traceparent` calls `parent-id` is not the parent, rather the +span ID. It has no field for the actual parent ID. + #### Debug flag B3 has always had a debug flag, which is a way to force a trace even if normal sampling says no. `traceparent` cannot distinguish between this and a normal decision, as it only has one bit flag. diff --git a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java new file mode 100644 index 0000000000..29c54c2975 --- /dev/null +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java @@ -0,0 +1,53 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import brave.propagation.Propagation; +import brave.propagation.Propagation.KeyFactory; +import brave.propagation.TraceContext; +import brave.propagation.TraceContext.Extractor; +import brave.propagation.TraceContext.Injector; +import java.util.LinkedHashMap; +import java.util.Map; +import org.junit.Test; + +import static brave.test.util.ClassLoaders.assertRunIsUnloadable; + +public class TraceContextPropagationClassLoaderTest { + @Test public void unloadable_afterBasicUsage() { + assertRunIsUnloadable(BasicUsage.class, getClass().getClassLoader()); + } + + static class BasicUsage implements Runnable { + @Override public void run() { + Propagation propagation = TraceContextPropagation.FACTORY.create(KeyFactory.STRING); + Injector> injector = propagation.injector(Map::put); + Extractor> extractor = propagation.extractor(Map::get); + + TraceContext context = TraceContext.newBuilder().traceId(1L).spanId(2L).build(); + + Map headers = new LinkedHashMap<>(); + injector.inject(context, headers); + + String traceparent = headers.get("traceparent"); + if (!"00-00000000000000000000000000000001-0000000000000002-00".equals(traceparent)) { + throw new AssertionError(); + } + + if (!context.equals(extractor.extract(headers).context())) { + throw new AssertionError(); + } + } + } +} From 4363d2d0942b54c426ff9435cd5e7a3835532d0d Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 18 Mar 2020 14:37:16 +0800 Subject: [PATCH 06/13] Starts adding tracestate validation --- .../TraceContextPropagationBenchmarks.java | 2 +- .../w3c/TracestateFormatBenchmarks.java | 58 ++++++++++ .../w3c/TraceContextExtractor.java | 6 +- .../propagation/w3c/TraceContextInjector.java | 6 +- .../w3c/TraceContextPropagation.java | 101 ++++++++++-------- .../propagation/w3c/TracestateFormat.java | 47 ++++++++ ...raceContextPropagationClassLoaderTest.java | 4 +- .../w3c/TraceContextPropagationTest.java | 61 ++++++----- .../propagation/w3c/TracestateFormatTest.java | 47 ++++++++ 9 files changed, 249 insertions(+), 83 deletions(-) create mode 100644 instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java create mode 100644 propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java 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 a119ede808..d4ff67b138 100644 --- a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TraceContextPropagationBenchmarks.java @@ -42,7 +42,7 @@ @OutputTimeUnit(TimeUnit.MICROSECONDS) public class TraceContextPropagationBenchmarks { static final Propagation tc = - TraceContextPropagation.FACTORY.create(Propagation.KeyFactory.STRING); + TraceContextPropagation.newFactory().create(Propagation.KeyFactory.STRING); static final Injector> tcInjector = tc.injector(Map::put); static final Extractor> tcExtractor = tc.extractor(Map::get); diff --git a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java new file mode 100644 index 0000000000..0da1cbe76b --- /dev/null +++ b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java @@ -0,0 +1,58 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import java.util.Arrays; +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +import static brave.propagation.w3c.TracestateFormat.validateKey; + +@Measurement(iterations = 5, time = 1) +@Warmup(iterations = 10, time = 1) +@Fork(3) +@BenchmarkMode(Mode.SampleTime) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +public class TracestateFormatBenchmarks { + static final String LONGEST; + + static { + char[] as = new char[256]; + Arrays.fill(as, 'a'); + LONGEST = new String(as); + } + + @Benchmark public boolean validateKey_range_longest() { + return validateKey(LONGEST); + } + + // Convenience main entry-point + public static void main(String[] args) throws RunnerException { + Options opt = new OptionsBuilder() + .include(".*" + TracestateFormatBenchmarks.class.getSimpleName()) + .build(); + + new Runner(opt).run(); + } +} 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 f5d1ed4497..ba914671dc 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java @@ -33,9 +33,9 @@ final class TraceContextExtractor implements Extractor { TraceContextExtractor(TraceContextPropagation propagation, Getter getter) { this.getter = getter; - this.traceparentKey = propagation.traceparentKey; - this.tracestateKey = propagation.tracestateKey; - this.tracestateFormat = new TracestateFormat(propagation.stateName); + this.traceparentKey = propagation.traceparent; + this.tracestateKey = propagation.tracestate; + this.tracestateFormat = new TracestateFormat(propagation.tracestateKey); } @Override public TraceContextOrSamplingFlags extract(C carrier) { 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 b0b6dc4d17..993e209f59 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java @@ -27,9 +27,9 @@ final class TraceContextInjector implements Injector { final K traceparentKey, tracestateKey; TraceContextInjector(TraceContextPropagation propagation, Setter setter) { - this.tracestateFormat = new TracestateFormat(propagation.stateName); - this.traceparentKey = propagation.traceparentKey; - this.tracestateKey = propagation.tracestateKey; + this.tracestateFormat = new TracestateFormat(propagation.tracestateKey); + this.traceparentKey = propagation.traceparent; + this.tracestateKey = propagation.tracestate; this.setter = setter; } 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 f539af5c78..c8cb3a99cb 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java @@ -21,52 +21,67 @@ import java.util.List; public final class TraceContextPropagation implements Propagation { - // TODO: not sure if we will want a constant here, or something like a builder. For example, we - // probably will need a primary state handler (ex b3) to catch data no longer in the traceparent - // format. At any rate, we will need to know what state is primarily ours, so that probably means - // not having a constant, unless that constant uses b3 single impl for the tracestate entry. - public static final Propagation.Factory FACTORY = - new Propagation.Factory() { - @Override public Propagation create(KeyFactory keyFactory) { - return new TraceContextPropagation<>(keyFactory); - } - - /** - * Traceparent doesn't support sharing the same span ID, though it may be possible to support - * this by extending it with a b3 state entry. - */ - @Override public boolean supportsJoin() { - return false; - } - - @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; - } - - @Override public String toString() { - return "TracestatePropagationFactory"; - } - }; - - final String stateName; - final K traceparentKey, tracestateKey; - final List fields; - - TraceContextPropagation(KeyFactory keyFactory) { - this.stateName = "b3"; - this.traceparentKey = keyFactory.create("traceparent"); - this.tracestateKey = keyFactory.create("tracestate"); - this.fields = Arrays.asList(traceparentKey, tracestateKey); + + public static Factory newFactory() { + return new FactoryBuilder().build(); + } + + public static FactoryBuilder newFactoryBuilder() { + return new FactoryBuilder(); + } + + public static final class FactoryBuilder { + String tracestateKey = "b3"; + + /** The key to use inside the {@code tracestate} value. Defaults to "b3". */ + public FactoryBuilder tracestateKey(String key) { + // https://w3c.github.io/trace-context/#key + if (key == null) throw new NullPointerException("key == null"); + // TODO: add an arg to throw with real message on invalid + if (!TracestateFormat.validateKey(key)) throw new IllegalArgumentException("invalid"); + this.tracestateKey = key; + return this; + } + + public Factory build() { + return new Factory(this); + } + + FactoryBuilder() { + } + } + + static final class Factory extends Propagation.Factory { + final String tracestateKey; + + Factory(FactoryBuilder builder) { + this.tracestateKey = builder.tracestateKey; + } + + @Override public Propagation create(KeyFactory keyFactory) { + return new TraceContextPropagation<>(keyFactory, tracestateKey); + } + + @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); + } + } + + 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 List keys() { - return fields; + return keys; } @Override public Injector injector(Setter setter) { 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 e8ab6f222e..72f2867dd0 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java @@ -121,4 +121,51 @@ String write(String thisValue, CharSequence otherEntries) { } return otherEntries; } + + // Simplify other rules by allowing value-based lookup on an ASCII value + static boolean[] VALID_KEY_CHARS = new boolean[128]; + + static { + for (char c = 0; c < 128; c++) { + VALID_KEY_CHARS[c] = isValidTracestateKeyChar(c); + } + } + + static boolean isValidTracestateKeyChar(char c) { + return (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') + || c == '@' || c == '_' || c == '-' || c == '*' || c == '/'; + } + + // TODO: add logging etc. + static boolean validateKey(CharSequence key) { + int length = key.length(); + if (length == 0 || length > 256) return false; + + // 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. + int vendorIndex = -1; + for (int i = 0; i < length; i++) { + char c = key.charAt(i); + + // The only valid characters are plain ASCII (numeric range 0-127) + if (c > 128 || !VALID_KEY_CHARS[c]) return false; + + if (c == '@') { + if (vendorIndex != -1) return false; + vendorIndex = i; + + // must be at least one character after the tenant + if (i++ == length) return false; + if (key.charAt(i) < 'a') return false; + } + } + + // 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 (vendorIndex == -1) return first >= 'a'; + + // tenant ID can only be up to 14 characters, but unlike vendor, it can start with a number. + return length - vendorIndex <= 14 && first >= 'a' || first <= '9'; + } } 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 29c54c2975..f829e04609 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationClassLoaderTest.java @@ -14,7 +14,6 @@ package brave.propagation.w3c; import brave.propagation.Propagation; -import brave.propagation.Propagation.KeyFactory; import brave.propagation.TraceContext; import brave.propagation.TraceContext.Extractor; import brave.propagation.TraceContext.Injector; @@ -22,6 +21,7 @@ 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,7 +31,7 @@ public class TraceContextPropagationClassLoaderTest { static class BasicUsage implements Runnable { @Override public void run() { - Propagation propagation = TraceContextPropagation.FACTORY.create(KeyFactory.STRING); + Propagation propagation = TraceContextPropagation.newFactory().create(STRING); Injector> injector = propagation.injector(Map::put); Extractor> extractor = propagation.extractor(Map::get); 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 0276171b56..8a99e7d902 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java @@ -13,7 +13,7 @@ */ package brave.propagation.w3c; -import brave.propagation.Propagation.KeyFactory; +import brave.propagation.Propagation; import brave.propagation.TraceContext; import brave.propagation.TraceContext.Extractor; import brave.propagation.TraceContext.Injector; @@ -24,23 +24,22 @@ import org.junit.Test; import static brave.internal.HexCodec.lowerHexToUnsignedLong; +import static brave.propagation.Propagation.KeyFactory.STRING; import static java.util.Arrays.asList; import static org.assertj.core.api.Assertions.assertThat; public class TraceContextPropagationTest { Map carrier = new LinkedHashMap<>(); - Injector> injector = - TraceContextPropagation.FACTORY.create(KeyFactory.STRING).injector(Map::put); - Extractor> extractor = - TraceContextPropagation.FACTORY.create(KeyFactory.STRING).extractor(Map::get); - - TraceContext sampledContext = - TraceContext.newBuilder() - .traceIdHigh(lowerHexToUnsignedLong("67891233abcdef01")) - .traceId(lowerHexToUnsignedLong("2345678912345678")) - .spanId(lowerHexToUnsignedLong("463ac35c9f6413ad")) - .sampled(true) - .build(); + Propagation propagation = TraceContextPropagation.newFactory().create(STRING); + Injector> injector = propagation.injector(Map::put); + Extractor> extractor = propagation.extractor(Map::get); + + TraceContext sampledContext = TraceContext.newBuilder() + .traceIdHigh(lowerHexToUnsignedLong("67891233abcdef01")) + .traceId(lowerHexToUnsignedLong("2345678912345678")) + .spanId(lowerHexToUnsignedLong("463ac35c9f6413ad")) + .sampled(true) + .build(); String validTraceparent = "00-67891233abcdef012345678912345678-463ac35c9f6413ad-01"; String validB3Single = "67891233abcdef012345678912345678-463ac35c9f6413ad-1"; String otherState = "congo=lZWRzIHRoNhcm5hbCBwbGVhc3VyZS4="; @@ -70,12 +69,12 @@ public class TraceContextPropagationTest { carrier.put("traceparent", validTraceparent); carrier.put("tracestate", "b3=" + validB3Single); - assertThat(extractor.extract(carrier)) - .isEqualTo( - TraceContextOrSamplingFlags.newBuilder() - .addExtra(new Extra()) - .context(sampledContext) - .build()); + assertThat(extractor.extract(carrier)).isEqualTo( + TraceContextOrSamplingFlags.newBuilder() + .addExtra(new Extra()) + .context(sampledContext) + .build() + ); } @Test public void extracts_b3_before_other_tracestate() { @@ -85,12 +84,12 @@ public class TraceContextPropagationTest { Extra extra = new Extra(); extra.otherEntries = otherState; - assertThat(extractor.extract(carrier)) - .isEqualTo( - TraceContextOrSamplingFlags.newBuilder() - .addExtra(extra) - .context(sampledContext) - .build()); + assertThat(extractor.extract(carrier)).isEqualTo( + TraceContextOrSamplingFlags.newBuilder() + .addExtra(extra) + .context(sampledContext) + .build() + ); } @Test public void extracts_b3_after_other_tracestate() { @@ -100,11 +99,11 @@ public class TraceContextPropagationTest { Extra extra = new Extra(); extra.otherEntries = otherState; - assertThat(extractor.extract(carrier)) - .isEqualTo( - TraceContextOrSamplingFlags.newBuilder() - .addExtra(extra) - .context(sampledContext) - .build()); + assertThat(extractor.extract(carrier)).isEqualTo( + TraceContextOrSamplingFlags.newBuilder() + .addExtra(extra) + .context(sampledContext) + .build() + ); } } diff --git a/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java b/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java new file mode 100644 index 0000000000..a25b1ecc21 --- /dev/null +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java @@ -0,0 +1,47 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import java.util.Arrays; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +public class TracestateFormatTest { + + // all these need log assertions + @Test public void validateKey_empty() { + assertThat(TracestateFormat.validateKey("")).isFalse(); + } + + @Test public void validateKey_unicode() { + String key = "a_💩_day"; + assertThat(TracestateFormat.validateKey(key)).isFalse(); + } + + @Test public void validateKey_tooLong() { + char[] tooMany = new char[257]; + assertThat(TracestateFormat.validateKey(new String(tooMany))).isFalse(); + } + + @Test public void validateKey_shortest() { + assertThat(TracestateFormat.validateKey("a")).isTrue(); + } + + @Test public void validateKey_longest() { + char[] longest = new char[256]; + Arrays.fill(longest, 'a'); + assertThat(TracestateFormat.validateKey(new String(longest))).isTrue(); + } +} From 9ac2ffd48b985b76f7279324fc8ee69a7e3a44f6 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 18 Mar 2020 15:16:38 +0800 Subject: [PATCH 07/13] adds benchmarks to show why not regex --- .../w3c/TracestateFormatBenchmarks.java | 46 +++++++++++++++---- .../propagation/w3c/TracestateFormatTest.java | 27 ++++++++--- 2 files changed, 58 insertions(+), 15 deletions(-) diff --git a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java index 0da1cbe76b..bc974aaa4b 100644 --- a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java @@ -13,8 +13,8 @@ */ package brave.propagation.w3c; -import java.util.Arrays; import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.BenchmarkMode; import org.openjdk.jmh.annotations.Fork; @@ -29,22 +29,50 @@ import static brave.propagation.w3c.TracestateFormat.validateKey; +/** + * This mainly shows the impact of much slower approaches, such as regular expressions. However, + * this is also used to help us evaluate efficiencies beyond that. + */ @Measurement(iterations = 5, time = 1) @Warmup(iterations = 10, time = 1) @Fork(3) -@BenchmarkMode(Mode.SampleTime) +@BenchmarkMode(Mode.Throughput) // simpler to interpret vs sample time @OutputTimeUnit(TimeUnit.MICROSECONDS) public class TracestateFormatBenchmarks { - static final String LONGEST; + // see https://github.com/w3c/trace-context/pull/386 for clearer definition of this stuff + static final String KEY_CHAR = "[a-z0-9_\\-*/]"; + static final Pattern KEY_PATTERN = Pattern.compile("^(" + + "[a-z]" + KEY_CHAR + "{0,255}" + // Basic Key + "|" + // OR + "[a-z0-9]" + KEY_CHAR + "{0,240}@[a-z]" + KEY_CHAR + "{0,13}" + // Tenant Key + ")$"); + + // copied from TracestateFormatTest as we don't share classpath + static final String FORTY_KEY_CHARS = "abcdefghijklmnopqrstuvwxyz0123456789_-*/"; + static final String TWO_HUNDRED_FORTY_KEY_CHARS = + FORTY_KEY_CHARS + FORTY_KEY_CHARS + FORTY_KEY_CHARS + + FORTY_KEY_CHARS + FORTY_KEY_CHARS + FORTY_KEY_CHARS; + + static final String LONGEST_BASIC_KEY = + TWO_HUNDRED_FORTY_KEY_CHARS + FORTY_KEY_CHARS.substring(0, 16); + + static final String LONGEST_TENANT_KEY = + "1" + TWO_HUNDRED_FORTY_KEY_CHARS + "@" + FORTY_KEY_CHARS.substring(0, 13); + + @Benchmark public boolean validateKey_brave_longest_basic() { + return validateKey(LONGEST_BASIC_KEY); + } + + @Benchmark public boolean validateKey_brave_longest_tenant() { + return validateKey(LONGEST_TENANT_KEY); + } - static { - char[] as = new char[256]; - Arrays.fill(as, 'a'); - LONGEST = new String(as); + @Benchmark public boolean validateKey_regex_longest_basic() { + return KEY_PATTERN.matcher(LONGEST_BASIC_KEY).matches(); } - @Benchmark public boolean validateKey_range_longest() { - return validateKey(LONGEST); + @Benchmark public boolean validateKey_regex_longest_tenant() { + return KEY_PATTERN.matcher(LONGEST_TENANT_KEY).matches(); } // Convenience main entry-point 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 a25b1ecc21..d1286a5b02 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java @@ -13,12 +13,21 @@ */ package brave.propagation.w3c; -import java.util.Arrays; import org.junit.Test; import static org.assertj.core.api.Assertions.assertThat; public class TracestateFormatTest { + static final String FORTY_KEY_CHARS = "abcdefghijklmnopqrstuvwxyz0123456789_-*/"; + static final String TWO_HUNDRED_FORTY_KEY_CHARS = + FORTY_KEY_CHARS + FORTY_KEY_CHARS + FORTY_KEY_CHARS + + FORTY_KEY_CHARS + FORTY_KEY_CHARS + FORTY_KEY_CHARS; + + static final String LONGEST_BASIC_KEY = + TWO_HUNDRED_FORTY_KEY_CHARS + FORTY_KEY_CHARS.substring(0, 16); + + static final String LONGEST_TENANT_KEY = + "1" + TWO_HUNDRED_FORTY_KEY_CHARS + "@" + FORTY_KEY_CHARS.substring(0, 13); // all these need log assertions @Test public void validateKey_empty() { @@ -35,13 +44,19 @@ public class TracestateFormatTest { assertThat(TracestateFormat.validateKey(new String(tooMany))).isFalse(); } - @Test public void validateKey_shortest() { + @Test public void validateKey_shortest_basic() { assertThat(TracestateFormat.validateKey("a")).isTrue(); } - @Test public void validateKey_longest() { - char[] longest = new char[256]; - Arrays.fill(longest, 'a'); - assertThat(TracestateFormat.validateKey(new String(longest))).isTrue(); + @Test public void validateKey_shortest_tenant() { + assertThat(TracestateFormat.validateKey("1@a")).isTrue(); + } + + @Test public void validateKey_longest_basic() { + assertThat(TracestateFormat.validateKey(LONGEST_BASIC_KEY)).isTrue(); + } + + @Test public void validateKey_longest_tenant() { + assertThat(TracestateFormat.validateKey(LONGEST_TENANT_KEY)).isTrue(); } } From 8d0e73beb895920523d4d71ffe58d7fc39f94133 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 18 Mar 2020 18:26:32 +0800 Subject: [PATCH 08/13] better perf and better tests --- .../propagation/w3c/TracestateFormat.java | 43 +++++++++++------- .../propagation/w3c/TracestateFormatTest.java | 44 ++++++++++++++++--- 2 files changed, 64 insertions(+), 23 deletions(-) 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 72f2867dd0..849d1ff4ac 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java @@ -122,11 +122,19 @@ String write(String thisValue, CharSequence otherEntries) { return otherEntries; } - // Simplify other rules by allowing value-based lookup on an ASCII value - static boolean[] VALID_KEY_CHARS = new boolean[128]; + // Simplify other 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 + // value as a key. + // + // The performance cost of a bounds check is still better than using BitSet, and avoids allocating + // an array of 64 thousand booleans: that could be problematic in old JREs or Android. + static int LAST_VALID_KEY_CHAR = 'z'; + static boolean[] VALID_KEY_CHARS = new boolean[LAST_VALID_KEY_CHAR + 1]; static { - for (char c = 0; c < 128; c++) { + for (char c = 0; c < VALID_KEY_CHARS.length; c++) { VALID_KEY_CHARS[c] = isValidTracestateKeyChar(c); } } @@ -142,30 +150,33 @@ static boolean validateKey(CharSequence key) { if (length == 0 || length > 256) return false; // 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. - int vendorIndex = -1; + // 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); - // The only valid characters are plain ASCII (numeric range 0-127) - if (c > 128 || !VALID_KEY_CHARS[c]) return false; + if (c > LAST_VALID_KEY_CHAR || !VALID_KEY_CHARS[c]) return false; if (c == '@') { - if (vendorIndex != -1) return false; - vendorIndex = i; - - // must be at least one character after the tenant - if (i++ == length) return false; - if (key.charAt(i) < 'a') return false; + if (atIndex != -1) return false; + atIndex = i; } } // 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 (vendorIndex == -1) return first >= 'a'; + if (atIndex == -1) return first >= 'a'; // <= 'z' implied + + // 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 || vendorLength > 14) return false; - // tenant ID can only be up to 14 characters, but unlike vendor, it can start with a number. - return length - vendorIndex <= 14 && first >= 'a' || first <= '9'; + return key.charAt(vendorIndex) >= 'a'; // <= 'z' implied + } else { + return false; // invalid tenant syntax + } } } 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 d1286a5b02..c5d2ca5a15 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java @@ -34,22 +34,18 @@ public class TracestateFormatTest { assertThat(TracestateFormat.validateKey("")).isFalse(); } - @Test public void validateKey_unicode() { - String key = "a_💩_day"; - assertThat(TracestateFormat.validateKey(key)).isFalse(); - } - @Test public void validateKey_tooLong() { char[] tooMany = new char[257]; assertThat(TracestateFormat.validateKey(new String(tooMany))).isFalse(); } @Test public void validateKey_shortest_basic() { - assertThat(TracestateFormat.validateKey("a")).isTrue(); + assertThat(TracestateFormat.validateKey("z")).isTrue(); } @Test public void validateKey_shortest_tenant() { - assertThat(TracestateFormat.validateKey("1@a")).isTrue(); + assertThat(TracestateFormat.validateKey("0@z")).isTrue(); + assertThat(TracestateFormat.validateKey("a@z")).isTrue(); } @Test public void validateKey_longest_basic() { @@ -59,4 +55,38 @@ public class TracestateFormatTest { @Test public void validateKey_longest_tenant() { assertThat(TracestateFormat.validateKey(LONGEST_TENANT_KEY)).isTrue(); } + + @Test public void validateKey_invalid_basic() { + // zero is allowed only as when there is an '@' + assertThat(TracestateFormat.validateKey("0")).isFalse(); + } + + @Test public void validateKey_invalid_basic_unicode() { + assertThat(TracestateFormat.validateKey("a💩")).isFalse(); + assertThat(TracestateFormat.validateKey("💩a")).isFalse(); + } + + @Test public void validateKey_invalid_tenant() { + assertThat(TracestateFormat.validateKey("_@z")).isFalse(); + } + + @Test public void validateKey_invalid_tenant_unicode() { + assertThat(TracestateFormat.validateKey("a@a💩")).isFalse(); + assertThat(TracestateFormat.validateKey("a@💩a")).isFalse(); + assertThat(TracestateFormat.validateKey("a💩@a")).isFalse(); + assertThat(TracestateFormat.validateKey("💩a@a")).isFalse(); + } + + @Test public void validateKey_invalid_tenant_empty() { + assertThat(TracestateFormat.validateKey("@a")).isFalse(); + assertThat(TracestateFormat.validateKey("a@")).isFalse(); + } + + @Test public void validateKey_invalid_tenant_vendor_longest() { + assertThat(TracestateFormat.validateKey("a@abcdef12345678")).isTrue(); + } + + @Test public void validateKey_invalid_tenant_vendor_tooLong() { + assertThat(TracestateFormat.validateKey("a@abcdef1234567890")).isFalse(); + } } From 8295d04b062ef3a7921a4677008d423de921ae42 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 18 Mar 2020 19:50:56 +0800 Subject: [PATCH 09/13] validates all the rules --- .../w3c/TracestateFormatBenchmarks.java | 4 +- .../w3c/TraceContextPropagation.java | 11 ++-- .../propagation/w3c/TracestateFormat.java | 54 +++++++++++++--- .../propagation/w3c/TracestateFormatTest.java | 63 +++++++++++++------ 4 files changed, 97 insertions(+), 35 deletions(-) diff --git a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java index bc974aaa4b..2c8396428d 100644 --- a/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/propagation/w3c/TracestateFormatBenchmarks.java @@ -60,11 +60,11 @@ public class TracestateFormatBenchmarks { "1" + TWO_HUNDRED_FORTY_KEY_CHARS + "@" + FORTY_KEY_CHARS.substring(0, 13); @Benchmark public boolean validateKey_brave_longest_basic() { - return validateKey(LONGEST_BASIC_KEY); + return validateKey(LONGEST_BASIC_KEY, false); } @Benchmark public boolean validateKey_brave_longest_tenant() { - return validateKey(LONGEST_TENANT_KEY); + return validateKey(LONGEST_TENANT_KEY, false); } @Benchmark public boolean validateKey_regex_longest_basic() { 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 c8cb3a99cb..472cfe76cf 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java @@ -33,12 +33,15 @@ public static FactoryBuilder newFactoryBuilder() { public static final class FactoryBuilder { String tracestateKey = "b3"; - /** The key to use inside the {@code tracestate} value. Defaults to "b3". */ + /** + * The key to use inside the {@code tracestate} value. Defaults to "b3". + * + * @throws IllegalArgumentException if the key doesn't conform to ABNF rules defined by the + * trace-context specification. + */ public FactoryBuilder tracestateKey(String key) { - // https://w3c.github.io/trace-context/#key if (key == null) throw new NullPointerException("key == null"); - // TODO: add an arg to throw with real message on invalid - if (!TracestateFormat.validateKey(key)) throw new IllegalArgumentException("invalid"); + TracestateFormat.validateKey(key, true); this.tracestateKey = key; return this; } 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 849d1ff4ac..5e8535d7a9 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java @@ -14,6 +14,7 @@ package brave.propagation.w3c; import brave.internal.Nullable; +import brave.internal.Platform; import static brave.propagation.w3c.TraceparentFormat.FORMAT_LENGTH; @@ -144,10 +145,18 @@ static boolean isValidTracestateKeyChar(char c) { || c == '@' || c == '_' || c == '-' || c == '*' || c == '/'; } - // TODO: add logging etc. - static boolean validateKey(CharSequence 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. + */ + static boolean validateKey(CharSequence key, boolean shouldThrow) { int length = key.length(); - if (length == 0 || length > 256) return false; + if (length == 0) return logOrThrow("Invalid input: empty", shouldThrow); + if (length > 256) return logOrThrow("Invalid input: too large", 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. @@ -155,10 +164,14 @@ static boolean validateKey(CharSequence key) { for (int i = 0; i < length; i++) { char c = key.charAt(i); - if (c > LAST_VALID_KEY_CHAR || !VALID_KEY_CHARS[c]) return false; + 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 false; + if (atIndex != -1) { + return logOrThrow("Invalid input: only a single '@' is allowed", shouldThrow); + } atIndex = i; } } @@ -166,17 +179,38 @@ static boolean validateKey(CharSequence key) { // 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 first >= 'a'; // <= 'z' implied + 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 + if ((first >= '0' && first <= '9') || first >= 'a') { // && <= 'z' implied int vendorIndex = atIndex + 1; int vendorLength = length - vendorIndex; - if (vendorLength == 0 || vendorLength > 14) return false; - return key.charAt(vendorIndex) >= 'a'; // <= 'z' implied + 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 false; // invalid tenant syntax + return logOrThrow("Invalid input: tenant ID must start with a-z", shouldThrow); + } + } + + 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); + } + + static boolean logOrThrow(String msg, boolean shouldThrow) { + if (shouldThrow) { + throw new IllegalArgumentException(msg); } + Platform.get().log(msg, null); + return false; } } 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 c5d2ca5a15..90f3a88339 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java @@ -13,9 +13,14 @@ */ 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; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TracestateFormatTest { static final String FORTY_KEY_CHARS = "abcdefghijklmnopqrstuvwxyz0123456789_-*/"; @@ -31,62 +36,82 @@ public class TracestateFormatTest { // all these need log assertions @Test public void validateKey_empty() { - assertThat(TracestateFormat.validateKey("")).isFalse(); + assertThatThrownByValidateKey("") + .hasMessage("Invalid input: empty"); } @Test public void validateKey_tooLong() { char[] tooMany = new char[257]; - assertThat(TracestateFormat.validateKey(new String(tooMany))).isFalse(); + Arrays.fill(tooMany, 'a'); + assertThatThrownByValidateKey(new String(tooMany)) + .hasMessage("Invalid input: too large"); } @Test public void validateKey_shortest_basic() { - assertThat(TracestateFormat.validateKey("z")).isTrue(); + assertThatValidateKey("z").isTrue(); } @Test public void validateKey_shortest_tenant() { - assertThat(TracestateFormat.validateKey("0@z")).isTrue(); - assertThat(TracestateFormat.validateKey("a@z")).isTrue(); + assertThatValidateKey("0@z").isTrue(); + assertThatValidateKey("a@z").isTrue(); } @Test public void validateKey_longest_basic() { - assertThat(TracestateFormat.validateKey(LONGEST_BASIC_KEY)).isTrue(); + assertThatValidateKey(LONGEST_BASIC_KEY).isTrue(); } @Test public void validateKey_longest_tenant() { - assertThat(TracestateFormat.validateKey(LONGEST_TENANT_KEY)).isTrue(); + assertThatValidateKey(LONGEST_TENANT_KEY).isTrue(); } @Test public void validateKey_invalid_basic() { // zero is allowed only as when there is an '@' - assertThat(TracestateFormat.validateKey("0")).isFalse(); + assertThatThrownByValidateKey("0") + .hasMessage("Invalid input: vendor must start with a-z"); } @Test public void validateKey_invalid_basic_unicode() { - assertThat(TracestateFormat.validateKey("a💩")).isFalse(); - assertThat(TracestateFormat.validateKey("💩a")).isFalse(); + Stream.of("a💩", "💩a").forEach(key -> assertThatThrownByValidateKey(key) + .hasMessage("Invalid input: valid characters are: a-z 0-9 _ - * / @")); } @Test public void validateKey_invalid_tenant() { - assertThat(TracestateFormat.validateKey("_@z")).isFalse(); + assertThatThrownByValidateKey("_@z") + .hasMessage("Invalid input: tenant ID must start with a-z"); } @Test public void validateKey_invalid_tenant_unicode() { - assertThat(TracestateFormat.validateKey("a@a💩")).isFalse(); - assertThat(TracestateFormat.validateKey("a@💩a")).isFalse(); - assertThat(TracestateFormat.validateKey("a💩@a")).isFalse(); - assertThat(TracestateFormat.validateKey("💩a@a")).isFalse(); + 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() { - assertThat(TracestateFormat.validateKey("@a")).isFalse(); - assertThat(TracestateFormat.validateKey("a@")).isFalse(); + assertThatThrownByValidateKey("@a") + .hasMessage("Invalid input: empty tenant ID"); + assertThatThrownByValidateKey("a@") + .hasMessage("Invalid input: empty vendor"); } @Test public void validateKey_invalid_tenant_vendor_longest() { - assertThat(TracestateFormat.validateKey("a@abcdef12345678")).isTrue(); + assertThatValidateKey("a@abcdef12345678").isTrue(); } @Test public void validateKey_invalid_tenant_vendor_tooLong() { - assertThat(TracestateFormat.validateKey("a@abcdef1234567890")).isFalse(); + assertThatThrownByValidateKey("a@abcdef1234567890") + .hasMessage("Invalid input: vendor too long"); + } + + static AbstractBooleanAssert assertThatValidateKey(String key) { + return assertThat(TracestateFormat.validateKey(key, true)); + } + + static AbstractThrowableAssert assertThatThrownByValidateKey(String key) { + return assertThatThrownBy(() -> TracestateFormat.validateKey(key, true)) + .isInstanceOf(IllegalArgumentException.class); } } From d409cf339ad81e6764ea7f5ab2de92709fc84c40 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 18 Mar 2020 20:01:44 +0800 Subject: [PATCH 10/13] polish --- .../brave/propagation/w3c/TracestateFormat.java | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) 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 5e8535d7a9..87ce2cbedf 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java @@ -153,6 +153,8 @@ static boolean isValidTracestateKeyChar(char c) { * @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); @@ -169,9 +171,7 @@ static boolean validateKey(CharSequence key, boolean shouldThrow) { } if (c == '@') { - if (atIndex != -1) { - return logOrThrow("Invalid input: only a single '@' is allowed", shouldThrow); - } + if (atIndex != -1) return logOrThrow("Invalid input: only one @ is allowed", shouldThrow); atIndex = i; } } @@ -179,9 +179,7 @@ static boolean validateKey(CharSequence key, boolean 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); - } + 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 @@ -207,9 +205,7 @@ static boolean validateVendorPrefix(char first, boolean shouldThrow) { } static boolean logOrThrow(String msg, boolean shouldThrow) { - if (shouldThrow) { - throw new IllegalArgumentException(msg); - } + if (shouldThrow) throw new IllegalArgumentException(msg); Platform.get().log(msg, null); return false; } From f5182b322ae27c0c67cd200780129695f2381b9c Mon Sep 17 00:00:00 2001 From: Samuel P Date: Sun, 12 Apr 2020 00:22:49 -0700 Subject: [PATCH 11/13] Trace context sync master and test fix (#1151) --- brave/src/main/java/brave/propagation/TraceContext.java | 2 +- propagation/pom.xml | 2 +- propagation/w3c/pom.xml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/brave/src/main/java/brave/propagation/TraceContext.java b/brave/src/main/java/brave/propagation/TraceContext.java index 1753b53a5a..2ea1214b82 100644 --- a/brave/src/main/java/brave/propagation/TraceContext.java +++ b/brave/src/main/java/brave/propagation/TraceContext.java @@ -518,7 +518,7 @@ public TraceContext build() { String missing = ""; if (traceIdHigh == 0L && traceId == 0L) missing += " traceId"; if (spanId == 0L) missing += " spanId"; - if (!"".equals(missing)) throw new IllegalArgumentException("Missing :" + missing); + if (!"".equals(missing)) throw new IllegalArgumentException("Missing:" + missing); return new TraceContext( flags, traceIdHigh, traceId, localRootId, parentId, spanId, ensureImmutable(extraList) ); diff --git a/propagation/pom.xml b/propagation/pom.xml index e5582c3979..13cf3bb23a 100644 --- a/propagation/pom.xml +++ b/propagation/pom.xml @@ -20,7 +20,7 @@ io.zipkin.brave brave-parent - 5.10.2-SNAPSHOT + 5.10.3-SNAPSHOT brave-propagation-parent diff --git a/propagation/w3c/pom.xml b/propagation/w3c/pom.xml index 4f975e78dd..700d504c19 100644 --- a/propagation/w3c/pom.xml +++ b/propagation/w3c/pom.xml @@ -18,7 +18,7 @@ io.zipkin.brave brave-propagation-parent - 5.10.2-SNAPSHOT + 5.10.3-SNAPSHOT 4.0.0 From 6537377cbd1564821a970002ef084513b83b6362 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Tue, 5 May 2020 09:16:44 +0800 Subject: [PATCH 12/13] polishing up --- propagation/pom.xml | 2 +- propagation/w3c/pom.xml | 2 +- .../w3c/TraceContextExtractor.java | 61 ++--- .../propagation/w3c/TraceContextInjector.java | 17 +- .../w3c/TraceContextPropagation.java | 19 +- .../propagation/w3c/TraceparentFormat.java | 19 +- .../brave/propagation/w3c/Tracestate.java | 50 ++++ .../propagation/w3c/TracestateFormat.java | 10 +- .../w3c/TraceContextPropagationTest.java | 116 ++++++---- .../w3c/TraceparentFormatTest.java | 219 +++++++++--------- .../propagation/w3c/TracestateFormatTest.java | 36 +-- 11 files changed, 290 insertions(+), 261 deletions(-) create mode 100644 propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java diff --git a/propagation/pom.xml b/propagation/pom.xml index 13cf3bb23a..e12bdd0bd6 100644 --- a/propagation/pom.xml +++ b/propagation/pom.xml @@ -20,7 +20,7 @@ io.zipkin.brave brave-parent - 5.10.3-SNAPSHOT + 5.11.3-SNAPSHOT brave-propagation-parent diff --git a/propagation/w3c/pom.xml b/propagation/w3c/pom.xml index 700d504c19..3f8b5947cb 100644 --- a/propagation/w3c/pom.xml +++ b/propagation/w3c/pom.xml @@ -18,7 +18,7 @@ io.zipkin.brave brave-propagation-parent - 5.10.3-SNAPSHOT + 5.11.3-SNAPSHOT 4.0.0 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 ba914671dc..6b2910d4d2 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java @@ -14,38 +14,37 @@ package brave.propagation.w3c; import brave.propagation.Propagation.Getter; -import brave.propagation.SamplingFlags; import brave.propagation.TraceContext; import brave.propagation.TraceContext.Extractor; import brave.propagation.TraceContextOrSamplingFlags; -import brave.propagation.w3c.TraceContextPropagation.Extra; -import java.util.Collections; -import java.util.List; import static brave.propagation.B3SingleFormat.parseB3SingleFormat; // TODO: this class has no useful tests wrt traceparent yet -final class TraceContextExtractor implements Extractor { - final Getter getter; +final class TraceContextExtractor implements Extractor { + static final TraceContextOrSamplingFlags EXTRACTED_EMPTY = + TraceContextOrSamplingFlags.EMPTY.toBuilder().addExtra(Tracestate.EMPTY).build(); + + 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); } - @Override public TraceContextOrSamplingFlags extract(C carrier) { - if (carrier == null) throw new NullPointerException("carrier == null"); - String traceparent = getter.get(carrier, traceparentKey); - if (traceparent == null) return EMPTY; + @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; // TODO: add link that says tracestate itself is optional - String tracestate = getter.get(carrier, tracestateKey); - if (tracestate == null) { + String tracestateString = getter.get(request, tracestateKey); + 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 // never be traced! @@ -54,33 +53,20 @@ final class TraceContextExtractor implements Extractor { // span ID. Ex we don't know if upstream are sending to the same system or not, when we can't // 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(traceparent); - return TraceContextOrSamplingFlags.newBuilder() - .context(maybeUpstream) - .extra(DEFAULT_EXTRA) // marker for outbound propagation - .build(); + TraceContext maybeUpstream = TraceparentFormat.parseTraceparentFormat(traceparentString); + return TraceContextOrSamplingFlags.newBuilder(maybeUpstream) + .addExtra(Tracestate.EMPTY) // marker for outbound propagation + .build(); } - CharSequence otherEntries = tracestateFormat.parseAndReturnOtherEntries(tracestate, handler); - - List extra; - if (otherEntries == null) { - extra = DEFAULT_EXTRA; - } else { - Extra e = new Extra(); - e.otherEntries = otherEntries; - extra = Collections.singletonList(e); - } + Tracestate tracestate = tracestateFormat.parseAndReturnOtherEntries(tracestateString, handler); TraceContext context = handler.context; if (context == null) { - if (extra == DEFAULT_EXTRA) return EMPTY; - return TraceContextOrSamplingFlags.newBuilder() - .extra(extra) - .samplingFlags(SamplingFlags.EMPTY) - .build(); + if (tracestate == Tracestate.EMPTY) return EXTRACTED_EMPTY; + return EXTRACTED_EMPTY.toBuilder().addExtra(tracestate).build(); } - return TraceContextOrSamplingFlags.newBuilder().context(context).extra(extra).build(); + return TraceContextOrSamplingFlags.newBuilder(context).addExtra(tracestate).build(); } static final class B3SingleFormatHandler implements TracestateFormat.Handler { @@ -93,11 +79,4 @@ public boolean onThisEntry(CharSequence tracestate, int beginIndex, int endIndex return context != null; } } - - /** When present, this context was created with TracestatePropagation */ - static final Extra MARKER = new Extra(); - - static final List DEFAULT_EXTRA = Collections.singletonList(MARKER); - static final TraceContextOrSamplingFlags EMPTY = - TraceContextOrSamplingFlags.EMPTY.toBuilder().extra(DEFAULT_EXTRA).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 993e209f59..2d9d0fe21e 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java @@ -16,37 +16,36 @@ import brave.propagation.Propagation.Setter; import brave.propagation.TraceContext; import brave.propagation.TraceContext.Injector; -import brave.propagation.w3c.TraceContextPropagation.Extra; import static brave.propagation.B3SingleFormat.writeB3SingleFormat; import static brave.propagation.w3c.TraceparentFormat.writeTraceparentFormat; -final class TraceContextInjector implements Injector { +final class TraceContextInjector implements Injector { final TracestateFormat tracestateFormat; - final Setter setter; + final Setter setter; final K traceparentKey, tracestateKey; - TraceContextInjector(TraceContextPropagation propagation, Setter setter) { + TraceContextInjector(TraceContextPropagation propagation, Setter setter) { this.tracestateFormat = new TracestateFormat(propagation.tracestateKey); this.traceparentKey = propagation.traceparent; this.tracestateKey = propagation.tracestate; this.setter = setter; } - @Override public void inject(TraceContext traceContext, C carrier) { + @Override public void inject(TraceContext traceContext, R request) { - setter.put(carrier, traceparentKey, writeTraceparentFormat(traceContext)); + 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 Extra) { - otherState = ((Extra) next).otherEntries; + if (next instanceof Tracestate) { + otherState = ((Tracestate) next).otherEntries; break; } } String tracestate = tracestateFormat.write(writeB3SingleFormat(traceContext), otherState); - setter.put(carrier, tracestateKey, tracestate); + setter.put(request, tracestateKey, tracestate); } } 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 472cfe76cf..84c4c32f0e 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java @@ -21,7 +21,6 @@ import java.util.List; public final class TraceContextPropagation implements Propagation { - public static Factory newFactory() { return new FactoryBuilder().build(); } @@ -87,27 +86,13 @@ static final class Factory extends Propagation.Factory { return keys; } - @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); } - - /** - * This only contains other entries. The entry for the current trace is only written during - * injection. - */ - static final class Extra { // hidden intentionally - CharSequence otherEntries; - - @Override public String toString() { - return "TracestatePropagation{" - + (otherEntries != null ? ("entries=" + otherEntries.toString()) : "") - + "}"; - } - } } 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 7060c88bea..2aabf11077 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java @@ -16,7 +16,6 @@ import brave.internal.Nullable; import brave.internal.Platform; import brave.propagation.TraceContext; -import java.lang.ref.WeakReference; import java.nio.ByteBuffer; import static brave.internal.HexCodec.writeHexLong; @@ -29,10 +28,10 @@ final class TraceparentFormat { static final int FORMAT_LENGTH = 3 + 32 + 1 + 16 + 3; // 00-traceid128-spanid-01 static final int // instead of enum for smaller bytecode - FIELD_VERSION = 1, - FIELD_TRACE_ID = 2, - FIELD_PARENT_ID = 3, - FIELD_TRACE_FLAGS = 4; + FIELD_VERSION = 1, + FIELD_TRACE_ID = 2, + FIELD_PARENT_ID = 3, + FIELD_TRACE_FLAGS = 4; /** Writes all "traceparent" defined fields in the trace context to a hyphen delimited string. */ public static String writeTraceparentFormat(TraceContext context) { @@ -42,7 +41,7 @@ public static String writeTraceparentFormat(TraceContext context) { } /** - * Like {@link #writeTraceparentFormat(TraceContext)}, but for carriers with byte array or byte + * Like {@link #writeTraceparentFormat(TraceContext)}, but for requests with byte array or byte * buffer values. For example, {@link ByteBuffer#wrap(byte[])} can wrap the result. */ public static byte[] writeTraceparentFormatAsBytes(TraceContext context) { @@ -89,7 +88,7 @@ public static TraceContext parseTraceparentFormat(CharSequence parent) { */ @Nullable public static TraceContext parseTraceparentFormat(CharSequence value, int beginIndex, - int endIndex) { + int endIndex) { int length = endIndex - beginIndex; if (length == 0) { @@ -214,9 +213,9 @@ public static TraceContext parseTraceparentFormat(CharSequence value, int beginI } static boolean validateFieldLength(int field, int length) { - int expectedLength = - (field == FIELD_TRACE_FLAGS || field == FIELD_VERSION) ? 2 - : field == FIELD_TRACE_ID ? 32 : 16; + int expectedLength = (field == FIELD_VERSION || field == FIELD_TRACE_FLAGS) + ? 2 // There are two fields that are 2 characters long: version and flags + : field == FIELD_TRACE_ID ? 32 : 16; // trace ID or span ID if (length == 0) { log(field, "Invalid input: empty {0}"); return false; diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java b/propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java new file mode 100644 index 0000000000..63125fe247 --- /dev/null +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java @@ -0,0 +1,50 @@ +/* + * Copyright 2013-2020 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.propagation.w3c; + +import brave.internal.Nullable; + +/** + * 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(""); + + // TODO: this will change + final String otherEntries; + + static Tracestate create(@Nullable CharSequence otherEntries) { + if (otherEntries == null || otherEntries.length() == 0) return EMPTY; + return new Tracestate(otherEntries.toString()); + } + + private Tracestate(String otherEntries) { + this.otherEntries = otherEntries; + } + + @Override public String toString() { + return "tracestate: " + otherEntries; + } + + @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 public final int hashCode() { + return otherEntries.hashCode(); + } +} \ 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 87ce2cbedf..58d1a72ba4 100644 --- a/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java +++ b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java @@ -13,7 +13,6 @@ */ package brave.propagation.w3c; -import brave.internal.Nullable; import brave.internal.Platform; import static brave.propagation.w3c.TraceparentFormat.FORMAT_LENGTH; @@ -79,7 +78,7 @@ String write(String thisValue, CharSequence otherEntries) { // 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= ' - @Nullable CharSequence parseAndReturnOtherEntries(String tracestate, Handler handler) { + Tracestate parseAndReturnOtherEntries(String tracestate, Handler handler) { StringBuilder currentString = new StringBuilder(), otherEntries = null; Op op; OUTER: @@ -120,7 +119,10 @@ String write(String thisValue, CharSequence otherEntries) { break; } } - return otherEntries; + 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. @@ -142,7 +144,7 @@ String write(String thisValue, CharSequence otherEntries) { static boolean isValidTracestateKeyChar(char c) { return (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') - || c == '@' || c == '_' || c == '-' || c == '*' || c == '/'; + || c == '@' || c == '_' || c == '-' || c == '*' || c == '/'; } /** 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 8a99e7d902..47eff3eeb1 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceContextPropagationTest.java @@ -18,92 +18,106 @@ import brave.propagation.TraceContext.Extractor; import brave.propagation.TraceContext.Injector; import brave.propagation.TraceContextOrSamplingFlags; -import brave.propagation.w3c.TraceContextPropagation.Extra; import java.util.LinkedHashMap; import java.util.Map; import org.junit.Test; import static brave.internal.HexCodec.lowerHexToUnsignedLong; import static brave.propagation.Propagation.KeyFactory.STRING; -import static java.util.Arrays.asList; import static org.assertj.core.api.Assertions.assertThat; public class TraceContextPropagationTest { - Map carrier = new LinkedHashMap<>(); + Map request = new LinkedHashMap<>(); Propagation propagation = TraceContextPropagation.newFactory().create(STRING); Injector> injector = propagation.injector(Map::put); Extractor> extractor = propagation.extractor(Map::get); TraceContext sampledContext = TraceContext.newBuilder() - .traceIdHigh(lowerHexToUnsignedLong("67891233abcdef01")) - .traceId(lowerHexToUnsignedLong("2345678912345678")) - .spanId(lowerHexToUnsignedLong("463ac35c9f6413ad")) - .sampled(true) - .build(); + .traceIdHigh(lowerHexToUnsignedLong("67891233abcdef01")) + .traceId(lowerHexToUnsignedLong("2345678912345678")) + .spanId(lowerHexToUnsignedLong("463ac35c9f6413ad")) + .sampled(true) + .build(); String validTraceparent = "00-67891233abcdef012345678912345678-463ac35c9f6413ad-01"; String validB3Single = "67891233abcdef012345678912345678-463ac35c9f6413ad-1"; String otherState = "congo=lZWRzIHRoNhcm5hbCBwbGVhc3VyZS4="; @Test public void injects_b3_when_no_other_tracestate() { - Extra extra = new Extra(); + sampledContext = sampledContext.toBuilder().addExtra(Tracestate.EMPTY).build(); - sampledContext = sampledContext.toBuilder().extra(asList(extra)).build(); + injector.inject(sampledContext, request); - injector.inject(sampledContext, carrier); - - assertThat(carrier).containsEntry("tracestate", "b3=" + validB3Single); + assertThat(request).containsEntry("tracestate", "b3=" + validB3Single); } @Test public void injects_b3_before_other_tracestate() { - Extra extra = new Extra(); - extra.otherEntries = otherState; + Tracestate tracestate = Tracestate.create(otherState); - sampledContext = sampledContext.toBuilder().extra(asList(extra)).build(); + sampledContext = sampledContext.toBuilder().addExtra(tracestate).build(); - injector.inject(sampledContext, carrier); + injector.inject(sampledContext, request); - assertThat(carrier).containsEntry("tracestate", "b3=" + validB3Single + "," + otherState); + assertThat(request).containsEntry("tracestate", "b3=" + validB3Single + "," + otherState); } @Test public void extracts_b3_when_no_other_tracestate() { - carrier.put("traceparent", validTraceparent); - carrier.put("tracestate", "b3=" + validB3Single); - - assertThat(extractor.extract(carrier)).isEqualTo( - TraceContextOrSamplingFlags.newBuilder() - .addExtra(new Extra()) - .context(sampledContext) - .build() - ); + request.put("traceparent", validTraceparent); + request.put("tracestate", "b3=" + validB3Single); + + assertThat(extractor.extract(request)).isEqualTo( + TraceContextOrSamplingFlags.newBuilder(sampledContext).addExtra(Tracestate.EMPTY).build()); } @Test public void extracts_b3_before_other_tracestate() { - carrier.put("traceparent", validTraceparent); - carrier.put("tracestate", "b3=" + validB3Single + "," + otherState); - - Extra extra = new Extra(); - extra.otherEntries = otherState; - - assertThat(extractor.extract(carrier)).isEqualTo( - TraceContextOrSamplingFlags.newBuilder() - .addExtra(extra) - .context(sampledContext) - .build() - ); + request.put("traceparent", validTraceparent); + request.put("tracestate", "b3=" + validB3Single + "," + otherState); + + Tracestate tracestate = Tracestate.create(otherState); + + assertThat(extractor.extract(request)).isEqualTo( + TraceContextOrSamplingFlags.newBuilder(sampledContext).addExtra(tracestate).build()); + } + + @Test public void extracted_toString() { + request.put("traceparent", validTraceparent); + request.put("tracestate", "b3=" + validB3Single + "," + otherState); + + assertThat(extractor.extract(request)).hasToString( + "Extracted{" + + "traceContext=" + sampledContext + ", " + + "samplingFlags=SAMPLED_REMOTE, " + + "extra=[tracestate: " + otherState + "]" + + "}"); } @Test public void extracts_b3_after_other_tracestate() { - carrier.put("traceparent", validTraceparent); - carrier.put("tracestate", otherState + ",b3=" + validB3Single); - - Extra extra = new Extra(); - extra.otherEntries = otherState; - - assertThat(extractor.extract(carrier)).isEqualTo( - TraceContextOrSamplingFlags.newBuilder() - .addExtra(extra) - .context(sampledContext) - .build() - ); + 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()); + } + + @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()); } } diff --git a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceparentFormatTest.java b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceparentFormatTest.java index 8870911c77..5d73a8e29f 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TraceparentFormatTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TraceparentFormatTest.java @@ -59,47 +59,47 @@ public class TraceparentFormatTest { /** unsampled isn't the same as not-yet-sampled, but we have no better choice */ @Test public void writeTraceparentFormat_notYetSampled_128() { TraceContext context = TraceContext.newBuilder() - .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)).build(); + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)).build(); assertThat(writeTraceparentFormat(context)) - .isEqualTo("00-" + traceIdHigh + traceId + "-" + spanId + "-00") - .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + .isEqualTo("00-" + traceIdHigh + traceId + "-" + spanId + "-00") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); } @Test public void writeTraceparentFormat_unsampled() { TraceContext context = TraceContext.newBuilder() - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(false).build(); + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(false).build(); assertThat(writeTraceparentFormat(context)) - .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-00") - .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-00") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); } @Test public void writeTraceparentFormat_sampled() { TraceContext context = TraceContext.newBuilder() - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(true).build(); + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build(); assertThat(writeTraceparentFormat(context)) - .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-01") - .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-01") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); } /** debug isn't the same as sampled, but we have no better choice */ @Test public void writeTraceparentFormat_debug() { TraceContext context = TraceContext.newBuilder() - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .debug(true).build(); + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .debug(true).build(); assertThat(writeTraceparentFormat(context)) - .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-01") - .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-01") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); } /** @@ -108,116 +108,116 @@ public class TraceparentFormatTest { */ @Test public void writeTraceparentFormat_parent() { TraceContext context = TraceContext.newBuilder() - .traceId(Long.parseUnsignedLong(traceId, 16)) - .parentId(Long.parseUnsignedLong(parentId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(true).build(); + .traceId(Long.parseUnsignedLong(traceId, 16)) + .parentId(Long.parseUnsignedLong(parentId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build(); assertThat(writeTraceparentFormat(context)) - .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-01") - .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + .isEqualTo("00-0000000000000000" + traceId + "-" + spanId + "-01") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); } @Test public void writeTraceparentFormat_largest() { TraceContext context = TraceContext.newBuilder() - .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) - .traceId(Long.parseUnsignedLong(traceId, 16)) - .parentId(Long.parseUnsignedLong(parentId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .debug(true).build(); + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .parentId(Long.parseUnsignedLong(parentId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .debug(true).build(); assertThat(writeTraceparentFormat(context)) - .isEqualTo("00-" + traceIdHigh + traceId + "-" + spanId + "-01") - .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); + .isEqualTo("00-" + traceIdHigh + traceId + "-" + spanId + "-01") + .isEqualTo(new String(writeTraceparentFormatAsBytes(context), UTF_8)); } @Test public void parseTraceparentFormat_sampled() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-01")) - .isEqualToComparingFieldByField(TraceContext.newBuilder() - .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(true).build() - ); + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); } @Test public void parseTraceparentFormat_unsampled() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-00")) - .isEqualToComparingFieldByField(TraceContext.newBuilder() - .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(false).build() - ); + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(false).build() + ); } @Test public void parseTraceparentFormat_padded() { assertThat(parseTraceparentFormat("00-0000000000000000" + traceId + "-" + spanId + "-01")) - .isEqualToComparingFieldByField(TraceContext.newBuilder() - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(true).build() - ); + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); } @Test public void parseTraceparentFormat_padded_right() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + "0000000000000000-" + spanId + "-01")) - .isEqualToComparingFieldByField(TraceContext.newBuilder() - .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(true).build() - ); + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); } @Test public void parseTraceparentFormat_newer_version() { assertThat(parseTraceparentFormat("10-" + traceIdHigh + traceId + "-" + spanId + "-00")) - .isEqualToComparingFieldByField(TraceContext.newBuilder() - .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(false).build() - ); + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(false).build() + ); } @Test public void parseTraceparentFormat_newer_version_ignores_extra_fields() { assertThat(parseTraceparentFormat("10-" + traceIdHigh + traceId + "-" + spanId + "-00-fobaly")) - .isEqualToComparingFieldByField(TraceContext.newBuilder() - .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(false).build() - ); + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(false).build() + ); } @Test public void parseTraceparentFormat_newer_version_ignores_extra_flags() { assertThat(parseTraceparentFormat("10-" + traceIdHigh + traceId + "-" + spanId + "-ff")) - .isEqualToComparingFieldByField(TraceContext.newBuilder() - .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(true).build() - ); + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); } /** for example, parsing inside tracestate */ @Test public void parseTraceparentFormat_middleOfString() { String input = "tc=00-" + traceIdHigh + traceId + "-" + spanId + "-01,"; assertThat(parseTraceparentFormat(input, 3, input.length() - 1)) - .isEqualToComparingFieldByField(TraceContext.newBuilder() - .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) - .traceId(Long.parseUnsignedLong(traceId, 16)) - .spanId(Long.parseUnsignedLong(spanId, 16)) - .sampled(true).build() - ); + .isEqualToComparingFieldByField(TraceContext.newBuilder() + .traceIdHigh(Long.parseUnsignedLong(traceIdHigh, 16)) + .traceId(Long.parseUnsignedLong(traceId, 16)) + .spanId(Long.parseUnsignedLong(spanId, 16)) + .sampled(true).build() + ); } @Test public void parseTraceparentFormat_middleOfString_incorrectIndex() { String input = "tc=00-" + traceIdHigh + traceId + "-" + spanId + "-00,"; assertThat(parseTraceparentFormat(input, 0, 12)) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform) - .log("Invalid input: only valid characters are lower-hex for {0}", "version", null); + .log("Invalid input: only valid characters are lower-hex for {0}", "version", null); } /** This tests that the being index is inclusive and the end index is exclusive */ @@ -225,74 +225,74 @@ public class TraceparentFormatTest { String encoded = "00-" + traceIdHigh + traceId + "-" + spanId + "-01"; String sequence = "??" + encoded + "??"; assertThat(parseTraceparentFormat(sequence, 2, 2 + encoded.length())) - .isEqualToComparingFieldByField(parseTraceparentFormat(encoded)); + .isEqualToComparingFieldByField(parseTraceparentFormat(encoded)); } @Test public void parseTraceparentFormat_malformed() { assertThat(parseTraceparentFormat("not-a-tumor")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform) - .log("Invalid input: only valid characters are lower-hex for {0}", "version", null); + .log("Invalid input: only valid characters are lower-hex for {0}", "version", null); } @Test public void parseTraceparentFormat_malformed_notAscii() { assertThat(parseTraceparentFormat( - "00-" + traceIdHigh + traceId + "-" + spanId.substring(0, 15) + "💩-1")) - .isNull(); // instead of crashing + "00-" + traceIdHigh + traceId + "-" + spanId.substring(0, 15) + "💩-1")) + .isNull(); // instead of crashing verify(platform) - .log("Invalid input: only valid characters are lower-hex for {0}", "parent ID", null); + .log("Invalid input: only valid characters are lower-hex for {0}", "parent ID", null); } @Test public void parseTraceparentFormat_malformed_uuid() { assertThat(parseTraceparentFormat("b970dafd-0d95-40aa-95d8-1d8725aebe40")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: {0} is too long", "version", null); } @Test public void parseTraceparentFormat_short_traceId() { assertThat( - parseTraceparentFormat("00-" + traceId + "-" + spanId + "-01")) - .isNull(); // instead of raising exception + parseTraceparentFormat("00-" + traceId + "-" + spanId + "-01")) + .isNull(); // instead of raising exception verify(platform).log("Invalid input: {0} is too short", "trace ID", null); } @Test public void parseTraceparentFormat_zero_traceId() { assertThat( - parseTraceparentFormat("00-00000000000000000000000000000000-" + spanId + "-01")) - .isNull(); // instead of raising exception + parseTraceparentFormat("00-00000000000000000000000000000000-" + spanId + "-01")) + .isNull(); // instead of raising exception verify(platform).log("Invalid input: read all zeros {0}", "trace ID", null); } @Test public void parseTraceparentFormat_fails_on_extra_flags() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-ff")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: only choices are 00 or 01 {0}", "trace flags", null); } @Test public void parseTraceparentFormat_fails_on_extra_fields() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-0-")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: {0} is too short", "trace flags", null); } @Test public void parseTraceparentFormat_fails_on_version_ff() { assertThat(parseTraceparentFormat("ff-" + traceIdHigh + traceId + "-" + spanId + "-01")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: ff {0}", "version", null); } @Test public void parseTraceparentFormat_zero_spanId() { assertThat( - parseTraceparentFormat("00-" + traceIdHigh + traceId + "-0000000000000000-01")) - .isNull(); // instead of raising exception + parseTraceparentFormat("00-" + traceIdHigh + traceId + "-0000000000000000-01")) + .isNull(); // instead of raising exception verify(platform).log("Invalid input: read all zeros {0}", "parent ID", null); } @@ -305,78 +305,79 @@ public class TraceparentFormatTest { @Test public void parseTraceparentFormat_empty_version() { assertThat(parseTraceparentFormat("-" + traceIdHigh + traceId + "-" + spanId + "-00")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: empty {0}", "version", null); } @Test public void parseTraceparentFormat_empty_traceId() { assertThat(parseTraceparentFormat("00--" + spanId + "-00")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: empty {0}", "trace ID", null); } @Test public void parseTraceparentFormat_empty_spanId() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "--01")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: empty {0}", "parent ID", null); } @Test public void parseTraceparentFormat_empty_flags() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: empty {0}", "trace flags", null); } @Test public void parseTraceparentFormat_truncated_traceId() { assertThat(parseTraceparentFormat("00-1-" + spanId + "-01")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: {0} is too short", "trace ID", null); } @Test public void parseTraceparentFormat_truncated_traceId128() { assertThat(parseTraceparentFormat("00-1" + traceId + "-" + spanId + "-01")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: {0} is too short", "trace ID", null); } @Test public void parseTraceparentFormat_truncated_spanId() { assertThat( - parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId.substring(0, 15) + "-00")) - .isNull(); // instead of raising exception + parseTraceparentFormat( + "00-" + traceIdHigh + traceId + "-" + spanId.substring(0, 15) + "-00")) + .isNull(); // instead of raising exception verify(platform).log("Invalid input: {0} is too short", "parent ID", null); } @Test public void parseTraceparentFormat_truncated_flags() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-0")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: {0} is too short", "trace flags", null); } @Test public void parseTraceparentFormat_traceIdTooLong() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "a" + "-" + spanId + "-0")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: {0} is too long", "trace ID", null); } @Test public void parseTraceparentFormat_spanIdTooLong() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "a-0")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: {0} is too long", "parent ID", null); } @Test public void parseTraceparentFormat_flagsTooLong() { assertThat(parseTraceparentFormat("00-" + traceIdHigh + traceId + "-" + spanId + "-001")) - .isNull(); // instead of raising exception + .isNull(); // instead of raising exception verify(platform).log("Invalid input: too long", null); } 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 90f3a88339..b31cccc599 100644 --- a/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java +++ b/propagation/w3c/src/test/java/brave/propagation/w3c/TracestateFormatTest.java @@ -25,26 +25,26 @@ public class TracestateFormatTest { static final String FORTY_KEY_CHARS = "abcdefghijklmnopqrstuvwxyz0123456789_-*/"; static final String TWO_HUNDRED_FORTY_KEY_CHARS = - FORTY_KEY_CHARS + FORTY_KEY_CHARS + FORTY_KEY_CHARS - + FORTY_KEY_CHARS + FORTY_KEY_CHARS + FORTY_KEY_CHARS; + FORTY_KEY_CHARS + FORTY_KEY_CHARS + FORTY_KEY_CHARS + + FORTY_KEY_CHARS + FORTY_KEY_CHARS + FORTY_KEY_CHARS; static final String LONGEST_BASIC_KEY = - TWO_HUNDRED_FORTY_KEY_CHARS + FORTY_KEY_CHARS.substring(0, 16); + TWO_HUNDRED_FORTY_KEY_CHARS + FORTY_KEY_CHARS.substring(0, 16); static final String LONGEST_TENANT_KEY = - "1" + TWO_HUNDRED_FORTY_KEY_CHARS + "@" + FORTY_KEY_CHARS.substring(0, 13); + "1" + TWO_HUNDRED_FORTY_KEY_CHARS + "@" + FORTY_KEY_CHARS.substring(0, 13); // all these need log assertions @Test public void validateKey_empty() { assertThatThrownByValidateKey("") - .hasMessage("Invalid input: empty"); + .hasMessage("Invalid input: empty"); } @Test public void validateKey_tooLong() { char[] tooMany = new char[257]; Arrays.fill(tooMany, 'a'); assertThatThrownByValidateKey(new String(tooMany)) - .hasMessage("Invalid input: too large"); + .hasMessage("Invalid input: too large"); } @Test public void validateKey_shortest_basic() { @@ -67,34 +67,34 @@ public class TracestateFormatTest { @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"); + .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 _ - * / @")); + .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"); + .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" + "a@a💩", + "a@💩a", + "a💩@a", + "💩a@a" ).forEach(key -> assertThatThrownByValidateKey(key) - .hasMessage("Invalid input: valid characters are: a-z 0-9 _ - * / @")); + .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"); + .hasMessage("Invalid input: empty tenant ID"); assertThatThrownByValidateKey("a@") - .hasMessage("Invalid input: empty vendor"); + .hasMessage("Invalid input: empty vendor"); } @Test public void validateKey_invalid_tenant_vendor_longest() { @@ -103,7 +103,7 @@ public class TracestateFormatTest { @Test public void validateKey_invalid_tenant_vendor_tooLong() { assertThatThrownByValidateKey("a@abcdef1234567890") - .hasMessage("Invalid input: vendor too long"); + .hasMessage("Invalid input: vendor too long"); } static AbstractBooleanAssert assertThatValidateKey(String key) { @@ -112,6 +112,6 @@ static AbstractBooleanAssert assertThatValidateKey(String key) { static AbstractThrowableAssert assertThatThrownByValidateKey(String key) { return assertThatThrownBy(() -> TracestateFormat.validateKey(key, true)) - .isInstanceOf(IllegalArgumentException.class); + .isInstanceOf(IllegalArgumentException.class); } } From beb345175c5ce593dbe43cc4e7b950665fb70c82 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 7 May 2020 14:28:11 +0800 Subject: [PATCH 13/13] 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); } }