diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index b67e335b8d..5aef217f67 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -18,7 +18,7 @@ can be automatically added by running `./mvnw com.mycila:license-maven-plugin:fo ``` /** - * Copyright 2019 The OpenZipkin Authors + * Copyright 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 diff --git a/HACKING.md b/HACKING.md index 27cef78ead..69ddb1252c 100644 --- a/HACKING.md +++ b/HACKING.md @@ -27,6 +27,9 @@ the intent of the feature are often merged and released in days. If a merged change isn't immediately released and it is of priority to you, nag (make a comment) on your merged pull request until it is released. +It is best to review rationale documents such as [brave/RATIONALE.md] +prior to raising a pull request. + ## How to experiment Changes to Brave's code are best addressed by the feature requestor in a pull request *after* discussing in an issue or on gitter. By discussing diff --git a/brave-tests/src/main/java/brave/test/propagation/PropagationTest.java b/brave-tests/src/main/java/brave/test/propagation/PropagationTest.java index f36de3b11c..ee51793343 100644 --- a/brave-tests/src/main/java/brave/test/propagation/PropagationTest.java +++ b/brave-tests/src/main/java/brave/test/propagation/PropagationTest.java @@ -13,7 +13,7 @@ */ package brave.test.propagation; -import brave.internal.HexCodec; +import brave.internal.codec.HexCodec; import brave.internal.Nullable; import brave.propagation.Propagation; import brave.propagation.Propagation.Getter; diff --git a/brave/RATIONALE.md b/brave/RATIONALE.md index 8530a9b311..7d3c03df8a 100644 --- a/brave/RATIONALE.md +++ b/brave/RATIONALE.md @@ -18,10 +18,57 @@ thought as it would betray productivity and make this document unreadable. Rationale here should be limited to impactful designs, and aspects non-obvious, non-conventional or subtle. -## Public namespace +## Java conventions Brave 4's public namespace is more defensive that the past, using a package accessor design from [OkHttp](https://github.com/square/okhttp). +We only expose types public internally or after significant demand. This keeps +the api small and easier to manage when charting migration paths. Otherwise, +types are always package private. + +Methods should only be marked public when they are intentional apis or +inheritance requires it. This practice prevents accidental dependence on +utilities. + +### Why no private symbols? (methods and fields) +Brave is a library with embedded use cases, such as inside Java agents or +Android code. + +For example, Android has a []hard limit on total methods in an application](https://developer.android.com/studio/build/multidex#avoid). +Fields marked private imply accessors in order to share state in the same +package. We routinely share state, such as sampling flag internals inside a +package. If we marked fields private, we'd count against that limit without +adding value. + +Modifiers on fields and methods are distracting to read and increase the size +of the bytecode generated during compilation. By recovering the size otherwise +spent on private modifiers, we not only avoid hitting limits, but we are also +able to add more code with the same jar size. + +For example, Brave 5.12 remains less than 250KiB, with no dependencies, all +features including deprecation bridges, and an embedded json serializer. + +This means we do not support sharing our packages with third parties, but we +do support an "always share inside a package" in our repository. In other +words, we trust our developers to proceed with caution. In the first seven +years of project history, we have had no issues raised with this policy. + +### Zero dependency policy +Brave is a telemetry library, which means it is used inside other low-level +libraries. We are unable to predict the version ranges of those libraries, and +attempting to do that would limit the applicability of Brave, which is an anti- +goal. Instead, we choose to use nothing except floor Java version features, +currently Java 6. + +### Why `new NullPointerException("xxx == null")` +For public entry points, we eagerly check null and throw `NullPointerException` +with a message like "xxx == null". This is not a normal pre-condition, such as +argument validation, which you'd throw `IllegalArgumentException` for. What's +happening here is we are making debugging (literally NPEs are bugs) easier, by +not deferring to Java to raise the NPE. If we deferred, it could be confusing +which local was null, especially as deferring results in an exception with no +message. + ## Stateful Span object Brave 4 allows you to pass around a Span object which can report itself to Zipkin when finished. This is better than using thread contexts in diff --git a/brave/src/main/java/brave/Tracing.java b/brave/src/main/java/brave/Tracing.java index f6b023553b..285299dcaa 100644 --- a/brave/src/main/java/brave/Tracing.java +++ b/brave/src/main/java/brave/Tracing.java @@ -16,7 +16,7 @@ import brave.baggage.BaggageField; import brave.handler.FinishedSpanHandler; import brave.handler.MutableSpan; -import brave.internal.IpLiteral; +import brave.internal.codec.IpLiteral; import brave.internal.Nullable; import brave.internal.Platform; import brave.internal.handler.NoopAwareFinishedSpanHandler; diff --git a/brave/src/main/java/brave/handler/MutableSpan.java b/brave/src/main/java/brave/handler/MutableSpan.java index 14265dedeb..2f44d00698 100644 --- a/brave/src/main/java/brave/handler/MutableSpan.java +++ b/brave/src/main/java/brave/handler/MutableSpan.java @@ -16,7 +16,7 @@ import brave.ErrorParser; import brave.Span.Kind; import brave.SpanCustomizer; -import brave.internal.IpLiteral; +import brave.internal.codec.IpLiteral; import brave.internal.Nullable; import brave.propagation.TraceContext; import java.lang.ref.WeakReference; @@ -24,7 +24,7 @@ import static brave.internal.InternalPropagation.FLAG_DEBUG; import static brave.internal.InternalPropagation.FLAG_SHARED; -import static brave.internal.JsonEscaper.jsonEscape; +import static brave.internal.codec.JsonEscaper.jsonEscape; /** * This represents a span except for its {@link TraceContext}. It is mutable, for late adjustments. diff --git a/brave/src/main/java/brave/internal/codec/EntrySplitter.java b/brave/src/main/java/brave/internal/codec/EntrySplitter.java new file mode 100644 index 0000000000..cb584ef661 --- /dev/null +++ b/brave/src/main/java/brave/internal/codec/EntrySplitter.java @@ -0,0 +1,312 @@ +/* + * 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.internal.codec; + +import brave.internal.Platform; + +/** + * Splits a character sequence that's in a delimited string, optionally trimming optional whitespace + * (OWS) before or after + * delimiters. + * + *

This is intended to be initialized as a constant, as doing so per-request will add + * unnecessary overhead. + */ +public final class EntrySplitter { + public static Builder newBuilder() { + return new Builder(); + } + + public static final class Builder { + int maxEntries = Integer.MAX_VALUE; + char entrySeparator = ',', keyValueSeparator = '='; + boolean trimOWSAroundEntrySeparator = true, trimOWSAroundKeyValueSeparator = true; + boolean keyValueSeparatorRequired = true, shouldThrow = false; + + /** + * When set, {@link Handler} will be called maximum {@code maxEntries} times per parse. After + * that, {@link #parse(Handler, Object, String)} returns false or throws an exception, based on + * {@link #shouldThrow(boolean)}. Default: {@link Integer#MAX_VALUE}. + * + *

This is used to implement strict format constraints. For example, above 32 entries is + * malformed. This is separate from any capacity constraints of the {@link Handler}, which may + * be smaller than this amount. + */ + public Builder maxEntries(int maxEntries) { + if (maxEntries <= 0) throw new IllegalArgumentException("maxEntries <= 0"); + this.maxEntries = maxEntries; + return this; + } + + /** + * The separator to use between entries. For example, the input "k1=v1,k2=v2", should have + * {@code entrySeparator} ','. Default: ',' + * + * @see #keyValueSeparator(char) + */ + public Builder entrySeparator(char entrySeparator) { + if (entrySeparator == 0) throw new IllegalArgumentException("entrySeparator == 0"); + this.entrySeparator = entrySeparator; + return this; + } + + /** + * The separator to use between a key and value. For example, the input "k1=v1,k2=v2" should + * have {@code keyValueSeparator} '='. Default: '=' + * + *

Note: Only the first {@code keyValueSeparator} identifies the end of the key + * until the next {@link #entrySeparator(char)}. This means values can include the {@code + * keyValueSeparator} character. + * + *

For example, the input "condition=animal=cat" with {@code keyValueSeparator} '=' parses + * {@code [("condition", "animal=cat")]} + * + * @see #keyValueSeparator(char) + */ + public Builder keyValueSeparator(char keyValueSeparator) { + if (keyValueSeparator == 0) throw new IllegalArgumentException("keyValueSeparator == 0"); + this.keyValueSeparator = keyValueSeparator; + return this; + } + + /** + * When {@code true}, optional whitespace (spaces and tabs aka OWS) + * are removed around the {@link #entrySeparator} and string boundaries. Default: {@code true} + * + *

For example, given the input " k1 = v1 , k2 = v2 ", this trims around the + * "=" character and string boundaries: {@code [("k1 "," v1"),("k2 ", " v2")]}. + * + * @see #trimOWSAroundKeyValueSeparator(boolean) + */ + public Builder trimOWSAroundEntrySeparator(boolean trimOWSAroundEntrySeparator) { + this.trimOWSAroundEntrySeparator = trimOWSAroundEntrySeparator; + return this; + } + + /** + * When {@code true}, optional whitespace (spaces and tabs aka OWS) + * are removed around the {@link #keyValueSeparator(char)}. Default: {@code true} + * + *

For example, given the input " k1 = v1 , k2 = v2 ", this trims around the + * "=" character and string boundaries: {@code [(" k1", "v1 "),(" k2", "v2 ")]}. + * + * @see #trimOWSAroundKeyValueSeparator(boolean) + */ + public Builder trimOWSAroundKeyValueSeparator(boolean trimOWSAroundKeyValueSeparator) { + this.trimOWSAroundKeyValueSeparator = trimOWSAroundKeyValueSeparator; + return this; + } + + /** + * When {@code true}, when a {@link #keyValueSeparator(char)} does not follow a key, {@link + * #parse(Handler, Object, String)} returns false or throws an exception, based on {@link + * #shouldThrow(boolean)}. Default: {@code true}. + * + *

Setting this to {@code false} makes "k1,k2=v2" interpreted the same as if there was + * a {@link #keyValueSeparator(char)}: "k1=,k2=v2". This is used for formats such as HTTP + * queries where separators are optional. + */ + public Builder keyValueSeparatorRequired(boolean keyValueSeparatorRequired) { + this.keyValueSeparatorRequired = keyValueSeparatorRequired; + return this; + } + + /** + * On validation fail, should this throw an exception or log?. The use case to throw is when + * validating input (ex into a builder), or in unit tests. + */ + public Builder shouldThrow(boolean shouldThrow) { + this.shouldThrow = shouldThrow; + return this; + } + + public EntrySplitter build() { + if (entrySeparator == keyValueSeparator) { + throw new IllegalArgumentException("entrySeparator == keyValueSeparator"); + } + return new EntrySplitter(this); + } + } + + /** + * This is a callback on offsets to avoid allocating strings for a malformed input {@code input}. + * + * @param target of parsed entries + */ + public interface Handler { + /** + * Called for each valid entry split from the input {@code input}. Return {@code false} after + * logging to stop due to invalid input. + * + *

After validating, typically strings will be parsed from the input like so: + * 

{@code
+     * String key = input.substring(beginKey, endKey);
+     * String value = input.substring(beginValue, endValue);
+     * }
+ * + * @param target receiver of parsed entries + * @param input string including data to parse + * @param beginKey begin index of the entry's key in {@code input}, inclusive + * @param endKey end index of the entry's key in {@code input}, exclusive + * @param beginValue begin index of the entry's value in {@code input}, inclusive + * @param endValue end index of the entry's value in {@code input}, exclusive + * @return true if we reached the {@code endIndex} without failures. + */ + boolean onEntry( + T target, String input, int beginKey, int endKey, int beginValue, int endValue); + } + + final char keyValueSeparator, entrySeparator; + int maxEntries; + final boolean trimOWSAroundEntrySeparator, trimOWSAroundKeyValueSeparator; + final boolean keyValueSeparatorRequired, shouldThrow; + final String missingKey, missingKeyValueSeparator, overMaxEntries; + + EntrySplitter(Builder builder) { + keyValueSeparator = builder.keyValueSeparator; + entrySeparator = builder.entrySeparator; + maxEntries = builder.maxEntries; + trimOWSAroundEntrySeparator = builder.trimOWSAroundEntrySeparator; + trimOWSAroundKeyValueSeparator = builder.trimOWSAroundKeyValueSeparator; + keyValueSeparatorRequired = builder.keyValueSeparatorRequired; + shouldThrow = builder.shouldThrow; + missingKey = "Invalid input: no key before '" + keyValueSeparator + "'"; + missingKeyValueSeparator = + "Invalid input: missing key value separator '" + keyValueSeparator + "'"; + overMaxEntries = "Invalid input: over " + maxEntries + " entries"; + } + + /** + * @param handler parses entries emitted upon success + * @param target receiver of parsed entries + * @param input string including data to parse + * @return true if we reached the {@code endIndex} without failures. + */ + public boolean parse(Handler handler, T target, String input) { + if (input == null) throw new NullPointerException("input == null"); + return parse(handler, target, input, 0, input.length()); + } + + /** + * @param handler parses entries emitted upon success + * @param target receiver of parsed entries + * @param input string including data to parse + * @param beginIndex begin index of the {@code input}, inclusive + * @param endIndex end index of the {@code input}, exclusive + * @return true if we reached the {@code endIndex} without failures. + */ + public boolean parse( + Handler handler, T target, String input, int beginIndex, int endIndex) { + if (handler == null) throw new NullPointerException("handler == null"); + if (target == null) throw new NullPointerException("target == null"); + if (input == null) throw new NullPointerException("input == null"); + if (beginIndex < 0) throw new IllegalArgumentException("beginIndex < 0"); + if (endIndex > input.length()) throw new IllegalArgumentException("endIndex > input.length()"); + + int remainingEntries = maxEntries, beginKey = -1, endKey = -1, beginValue = -1; + for (int i = beginIndex; i < endIndex; i++) { + char c = input.charAt(i); + + boolean nextIsEnd = i + 1 == endIndex; + if (c == entrySeparator || nextIsEnd) { // finished an entry + if (c == keyValueSeparator) { + beginValue = i; // key separator at end of the input. ex "key=" or "k1 =", but not "k1" + } + + if (beginKey == -1 && beginValue == -1) { + continue; // ignore empty entries, like ",," + } else if (beginKey == -1) { + return logOrThrow(missingKey, shouldThrow); // ex. "=" ",=" + } else if (nextIsEnd && beginValue == -1) { + // We reached the end of a key-only entry, a single character entry or an empty entry + // at the end of the input. ex "k1" "k1 " "a=b" "..=," + beginValue = c == entrySeparator ? i + 1 : i; + } + + int endValue; + if (endKey == -1) { + if (keyValueSeparatorRequired && c != keyValueSeparator) { + return logOrThrow(missingKeyValueSeparator, shouldThrow); // throw on "k1" "k1=v2,k2" + } + + // Even though we have an empty value, we need to handle whitespace and + // boundary conditions. + // + // For example, using entry separator ',' and KV separator '=': + // "...,k1" and input[i] == 'y', we want i + 1, so that the key includes the 'y' + // "...,k1 " and input[i] == ' ', we want i + 1, as the key includes a trailing ' ' + // "...,k1=" and input[i] == '=', we want i, bc a KV separator isn't part of the key + // "k1 , k2" and input[i] == ',', we want i, bc an entry separator isn't part of the key + endKey = nextIsEnd && c != keyValueSeparator ? i + 1 : i; + + if (trimOWSAroundKeyValueSeparator) { + endKey = rewindOWS(input, beginKey, endKey); + } + beginValue = endValue = endKey; // value is empty + } else { + endValue = nextIsEnd ? i + 1 : i; + + if (trimOWSAroundEntrySeparator) { + endValue = rewindOWS(input, beginValue, endValue); + } + } + + if (remainingEntries-- == 0) logOrThrow(overMaxEntries, shouldThrow); + + if (!handler.onEntry(target, input, beginKey, endKey, beginValue, endValue)) { + return false; // assume handler logs + } + + beginKey = endKey = beginValue = -1; // reset for the next entry + } else if (beginKey == -1) { + if (trimOWSAroundEntrySeparator && isOWS(c)) continue; // skip whitespace before key + if (c == keyValueSeparator) { + if (i == beginIndex || input.charAt(i - 1) == entrySeparator) { + return logOrThrow(missingKey, shouldThrow); // ex "=v1" ",=v2" + } + } + beginKey = i; + } else if (endKey == -1 && c == keyValueSeparator) { // only use the first separator for key + endKey = i; + if (trimOWSAroundKeyValueSeparator) { + endKey = rewindOWS(input, beginIndex, endKey); + } + } else if (endKey != -1 && beginValue == -1) { // only look for a value if you have a key + if (trimOWSAroundKeyValueSeparator && isOWS(c)) continue; // skip whitespace before value + if (c == keyValueSeparator) continue; // skip the keyValueSeparator (ex '=') + beginValue = i; + } + } + return true; + } + + static int rewindOWS(String input, int beginIndex, int endIndex) { + // endIndex is a boundary. we need to begin looking one character before it. + while (isOWS(input.charAt(endIndex - 1))) { + if (--endIndex == beginIndex) return beginIndex; // trim whitespace + } + return endIndex; + } + + // OWS is zero or more spaces or tabs https://httpwg.org/specs/rfc7230.html#rfc.section.3.2 + static boolean isOWS(char c) { + return c == ' ' || c == '\t'; + } + + static boolean logOrThrow(String msg, boolean shouldThrow) { + if (shouldThrow) throw new IllegalArgumentException(msg); + Platform.get().log(msg, null); + return false; + } +} diff --git a/brave/src/main/java/brave/internal/HexCodec.java b/brave/src/main/java/brave/internal/codec/HexCodec.java similarity index 98% rename from brave/src/main/java/brave/internal/HexCodec.java rename to brave/src/main/java/brave/internal/codec/HexCodec.java index 26706c6947..1db67d2c31 100644 --- a/brave/src/main/java/brave/internal/HexCodec.java +++ b/brave/src/main/java/brave/internal/codec/HexCodec.java @@ -11,7 +11,9 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ -package brave.internal; +package brave.internal.codec; + +import brave.internal.RecyclableBuffers; // code originally imported from zipkin.Util public final class HexCodec { diff --git a/brave/src/main/java/brave/internal/IpLiteral.java b/brave/src/main/java/brave/internal/codec/IpLiteral.java similarity index 98% rename from brave/src/main/java/brave/internal/IpLiteral.java rename to brave/src/main/java/brave/internal/codec/IpLiteral.java index 073bf654cd..222d7fa74e 100644 --- a/brave/src/main/java/brave/internal/IpLiteral.java +++ b/brave/src/main/java/brave/internal/codec/IpLiteral.java @@ -11,7 +11,9 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ -package brave.internal; +package brave.internal.codec; + +import brave.internal.Nullable; /** Internal utility class to validate IPv4 or IPv6 literals */ public final class IpLiteral { diff --git a/brave/src/main/java/brave/internal/JsonEscaper.java b/brave/src/main/java/brave/internal/codec/JsonEscaper.java similarity index 98% rename from brave/src/main/java/brave/internal/JsonEscaper.java rename to brave/src/main/java/brave/internal/codec/JsonEscaper.java index 84d103306c..7fde03b97c 100644 --- a/brave/src/main/java/brave/internal/JsonEscaper.java +++ b/brave/src/main/java/brave/internal/codec/JsonEscaper.java @@ -11,7 +11,7 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ -package brave.internal; +package brave.internal.codec; // Initially, a copy of zipkin2.internal.JsonEscaper public final class JsonEscaper { diff --git a/brave/src/main/java/brave/propagation/B3SingleFormat.java b/brave/src/main/java/brave/propagation/B3SingleFormat.java index c3998342e4..70a95232ef 100644 --- a/brave/src/main/java/brave/propagation/B3SingleFormat.java +++ b/brave/src/main/java/brave/propagation/B3SingleFormat.java @@ -19,7 +19,7 @@ import java.nio.ByteBuffer; import java.util.Collections; -import static brave.internal.HexCodec.writeHexLong; +import static brave.internal.codec.HexCodec.writeHexLong; /** * This format corresponds to the propagation key "b3" (or "B3"), which delimits fields in the diff --git a/brave/src/main/java/brave/propagation/TraceContext.java b/brave/src/main/java/brave/propagation/TraceContext.java index fcf0c97956..b5066e8cab 100644 --- a/brave/src/main/java/brave/propagation/TraceContext.java +++ b/brave/src/main/java/brave/propagation/TraceContext.java @@ -21,9 +21,9 @@ import java.util.Collections; import java.util.List; -import static brave.internal.HexCodec.lenientLowerHexToUnsignedLong; -import static brave.internal.HexCodec.toLowerHex; -import static brave.internal.HexCodec.writeHexLong; +import static brave.internal.codec.HexCodec.lenientLowerHexToUnsignedLong; +import static brave.internal.codec.HexCodec.toLowerHex; +import static brave.internal.codec.HexCodec.writeHexLong; import static brave.internal.InternalPropagation.FLAG_LOCAL_ROOT; import static brave.internal.InternalPropagation.FLAG_SAMPLED; import static brave.internal.InternalPropagation.FLAG_SAMPLED_LOCAL; diff --git a/brave/src/main/java/brave/propagation/TraceIdContext.java b/brave/src/main/java/brave/propagation/TraceIdContext.java index 6bfe8b36e8..3d2306f993 100644 --- a/brave/src/main/java/brave/propagation/TraceIdContext.java +++ b/brave/src/main/java/brave/propagation/TraceIdContext.java @@ -17,8 +17,8 @@ import brave.internal.Nullable; import brave.internal.RecyclableBuffers; -import static brave.internal.HexCodec.toLowerHex; -import static brave.internal.HexCodec.writeHexLong; +import static brave.internal.codec.HexCodec.toLowerHex; +import static brave.internal.codec.HexCodec.writeHexLong; import static brave.internal.InternalPropagation.FLAG_SAMPLED; import static brave.internal.InternalPropagation.FLAG_SAMPLED_SET; diff --git a/brave/src/test/java/brave/features/baggage/SingleHeaderCodec.java b/brave/src/test/java/brave/features/baggage/SingleHeaderCodec.java index b73abbc767..029efc4ef2 100644 --- a/brave/src/test/java/brave/features/baggage/SingleHeaderCodec.java +++ b/brave/src/test/java/brave/features/baggage/SingleHeaderCodec.java @@ -16,6 +16,7 @@ import brave.baggage.BaggageField; import brave.baggage.BaggageField.ValueUpdater; import brave.internal.baggage.BaggageCodec; +import brave.internal.codec.EntrySplitter; import brave.propagation.TraceContext; import java.util.Collections; import java.util.List; @@ -26,7 +27,8 @@ * *

See https://github.com/w3c/correlation-context/blob/master/correlation_context/HTTP_HEADER_FORMAT.md */ -final class SingleHeaderCodec implements BaggageCodec { +final class SingleHeaderCodec implements BaggageCodec, EntrySplitter.Handler { + static final EntrySplitter ENTRY_SPLITTER = EntrySplitter.newBuilder().build(); static final SingleHeaderCodec INSTANCE = new SingleHeaderCodec(); static BaggageCodec get() { @@ -45,12 +47,14 @@ static BaggageCodec get() { } @Override public boolean decode(ValueUpdater valueUpdater, Object request, String value) { - boolean decoded = false; - for (String entry : value.split(",", -1)) { - String[] keyValue = entry.split("=", 2); - if (valueUpdater.updateValue(BaggageField.create(keyValue[0]), keyValue[1])) decoded = true; - } - return decoded; + return ENTRY_SPLITTER.parse(this, valueUpdater, value); + } + + @Override public boolean onEntry( + ValueUpdater target, String buffer, int beginKey, int endKey, int beginValue, int endValue) { + BaggageField field = BaggageField.create(buffer.substring(beginKey, endKey)); + String value = buffer.substring(beginValue, endValue); + return target.updateValue(field, value); } @Override public String encode(Map values, TraceContext context, Object request) { diff --git a/brave/src/test/java/brave/internal/PlatformTest.java b/brave/src/test/java/brave/internal/PlatformTest.java index c89c3c172c..23daf32cf7 100644 --- a/brave/src/test/java/brave/internal/PlatformTest.java +++ b/brave/src/test/java/brave/internal/PlatformTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2019 The OpenZipkin Authors + * 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 @@ -13,6 +13,7 @@ */ package brave.internal; +import brave.internal.codec.HexCodec; import com.google.common.collect.Sets; import java.net.Inet6Address; import java.net.InetAddress; diff --git a/brave/src/test/java/brave/internal/codec/EntrySplitterTest.java b/brave/src/test/java/brave/internal/codec/EntrySplitterTest.java new file mode 100644 index 0000000000..bd6d334432 --- /dev/null +++ b/brave/src/test/java/brave/internal/codec/EntrySplitterTest.java @@ -0,0 +1,388 @@ +/* + * 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.internal.codec; + +import brave.internal.codec.EntrySplitter.Handler; +import brave.propagation.TraceContext; +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.entry; + +public class EntrySplitterTest { + EntrySplitter entrySplitter = EntrySplitter.newBuilder().shouldThrow(true).build(); + Map map = new LinkedHashMap<>(); + Handler> parseIntoMap = + (target, input, beginKey, endKey, beginValue, endValue) -> { + String key = input.substring(beginKey, endKey); + String value = input.substring(beginValue, endValue); + target.put(key, value); + return true; + }; + + @Test public void parse() { + entrySplitter.parse(parseIntoMap, map, "k1=v1,k2=v2"); + + assertThat(map).containsExactly( + entry("k1", "v1"), + entry("k2", "v2") + ); + } + + @Test public void parse_singleChars() { + entrySplitter.parse(parseIntoMap, map, "k=v,a=b"); + + assertThat(map).containsExactly( + entry("k", "v"), + entry("a", "b") + ); + } + + @Test public void parse_valuesAreRequired() { + for (String missingValue : Arrays.asList("k1", "k1 ", "k1=v1,k2", "k1 ,k2=v1")) { + assertThatThrownBy(() -> entrySplitter.parse(parseIntoMap, map, missingValue)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Invalid input: missing key value separator '='"); + } + assertThat(map.isEmpty()); + } + + @Test public void parse_emptyValuesOk() { + for (String emptyValue : Arrays.asList("k1=", "k1 =", ",k1=", ",k1 =", "k1 =,")) { + entrySplitter.parse(parseIntoMap, map, emptyValue); + + assertThat(map).containsExactly(entry("k1", "")); + map.clear(); + } + + entrySplitter.parse(parseIntoMap, map, "k1=v1,k2="); + + assertThat(map).containsExactly( + entry("k1", "v1"), + entry("k2", "") + ); + } + + @Test public void keyValueSeparatorRequired_false() { + entrySplitter = EntrySplitter.newBuilder() + .keyValueSeparatorRequired(false) + .shouldThrow(true) + .build(); + + entrySplitter.parse(parseIntoMap, map, " authcache , gateway "); + + assertThat(map).containsExactly( + entry("authcache", ""), + entry("gateway", "") + ); + } + + /** Parse Accept header style encoding as used in secondary sampling */ + @Test public void parse_onlyFirstKeyValueSeparator() { + entrySplitter = EntrySplitter.newBuilder() + .keyValueSeparator(';') + .keyValueSeparatorRequired(false) + .shouldThrow(true) + .build(); + + entrySplitter.parse(parseIntoMap, map, "authcache;ttl=1;spanId=19f84f102048e047,gateway"); + + assertThat(map).containsExactly( + entry("authcache", "ttl=1;spanId=19f84f102048e047"), + entry("gateway", "") + ); + } + + /** This shows you can nest parsers without unnecessary string allocation between stages. */ + @Test public void parse_nested() { + EntrySplitter outerSplitter = EntrySplitter.newBuilder() + .keyValueSeparator(';') + .keyValueSeparatorRequired(false) + .shouldThrow(true) + .build(); + + EntrySplitter innerSplitter = EntrySplitter.newBuilder() + .entrySeparator(';') + .keyValueSeparator('=') + .shouldThrow(true) + .build(); + + Map> keyToAttributes = new LinkedHashMap<>(); + + outerSplitter.parse((target, input, beginKey, endKey, beginValue, endValue) -> { + String key = input.substring(beginKey, endKey); + Map attributes = new LinkedHashMap<>(); + if (beginValue == endValue) { // no string allocation at all + attributes = Collections.emptyMap(); + } else { // no string allocation to pass to the inner parser + attributes = new LinkedHashMap<>(); + innerSplitter.parse(parseIntoMap, attributes, input, beginValue, endValue); + } + target.put(key, attributes); + return true; + }, keyToAttributes, "authcache;ttl=1;spanId=19f84f102048e047,gateway"); + + Map expectedAttributes = new LinkedHashMap<>(); + expectedAttributes.put("ttl", "1"); + expectedAttributes.put("spanId", "19f84f102048e047"); + assertThat(keyToAttributes).containsExactly( + entry("authcache", expectedAttributes), + entry("gateway", Collections.emptyMap()) + ); + } + + @Test public void parse_emptyKeysNotOk() { + for (String missingKey : Arrays.asList("=", "=v1", ",=", ",=v2")) { + assertThatThrownBy(() -> entrySplitter.parse(parseIntoMap, map, missingKey)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Invalid input: no key before '='"); + } + } + + /** + * This is an example of how to parse without allocating strings. This is based on + * https://github.com/openzipkin/zipkin-aws/blob/master/brave-propagation-aws/src/main/java/brave/propagation/aws/AWSPropagation.java + */ + @Test public void example_parseAWSTraceId() { + entrySplitter = EntrySplitter.newBuilder().entrySeparator(';').build(); + + String awsTraceId = + "Root=1-67891233-abcdef012345678912345678;Parent=463ac35c9f6413ad;Sampled=1"; + + Handler parseIntoMap = + (target, input, beginKey, endKey, beginValue, endValue) -> { + int keyLength = endKey - beginKey; + if (input.regionMatches(beginKey, "Root", 0, keyLength)) { + int valueLength = endValue - beginValue; + int i = beginValue; + if (valueLength != 35 // length of 1-67891233-abcdef012345678912345678 + || input.charAt(i++) != '1' + || input.charAt(i++) != '-') { + return false; // invalid version or format + } + long high32 = HexCodec.lenientLowerHexToUnsignedLong(input, i, i + 8); + i += 9; // skip the hyphen + long low32 = HexCodec.lenientLowerHexToUnsignedLong(input, i, i + 8); + i += 8; + long traceIdHigh = high32 << 32; + traceIdHigh = traceIdHigh | low32; + long traceId = HexCodec.lenientLowerHexToUnsignedLong(input, i, i + 16); + if (traceIdHigh == 0L || traceId == 0L) return false; + target.traceIdHigh(traceIdHigh).traceId(traceId); + return true; + } else if (input.regionMatches(beginKey, "Parent", 0, keyLength)) { + long spanId = HexCodec.lenientLowerHexToUnsignedLong(input, beginValue, endValue); + if (spanId == 0L) return false; + target.spanId(spanId); + return true; + } else if (input.regionMatches(beginKey, "Sampled", 0, keyLength)) { + switch (input.charAt(beginValue)) { + case '0': + target.sampled(false); + break; + case '1': + target.sampled(true); + break; + default: + return false; + } + } + return true; + }; + + TraceContext.Builder builder = TraceContext.newBuilder(); + assertThat(entrySplitter.parse(parseIntoMap, builder, awsTraceId)).isTrue(); + TraceContext context = builder.build(); + + assertThat(context).usingRecursiveComparison().isEqualTo( + TraceContext.newBuilder() + .traceIdHigh(0x67891233abcdef01L).traceId(0x2345678912345678L) + .spanId(0x463ac35c9f6413adL) + .sampled(true) + .build() + ); + } + + @Test public void parse_breaksWhenHandlerDoes() { + entrySplitter = EntrySplitter.newBuilder().maxEntries(2).shouldThrow(true).build(); + + entrySplitter.parse((target, input, beginKey, endKey, beginValue, endValue) -> { + String key = input.substring(beginKey, endKey); + if (key.equals("k1")) { + target.put(key, input.substring(beginValue, endValue)); + return true; + } + return false; + }, map, "k1=v1,k2=v2"); + + assertThat(map).containsExactly(entry("k1", "v1")); + } + + @Test public void parse_maxEntries() { + entrySplitter = EntrySplitter.newBuilder().maxEntries(2).shouldThrow(true).build(); + + entrySplitter.parse(parseIntoMap, map, "k1=v1,k2=v2"); + + assertThat(map).containsExactly( + entry("k1", "v1"), + entry("k2", "v2") + ); + + assertThatThrownBy(() -> entrySplitter.parse(parseIntoMap, map, "k1=v1,k2=v2,k3=v3")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Invalid input: over 2 entries"); + } + + @Test public void parse_whitespaceInKeyValue() { + entrySplitter.parse(parseIntoMap, map, "k 1=v 1,k 2=v 2"); + + assertThat(map).containsExactly( + entry("k 1", "v 1"), + entry("k 2", "v 2") + ); + } + + @Test public void trimOWSAroundEntrySeparator() { + entrySplitter = EntrySplitter.newBuilder() + .trimOWSAroundEntrySeparator(true) + .trimOWSAroundKeyValueSeparator(false) + .shouldThrow(true).build(); + + entrySplitter.parse(parseIntoMap, map, " k1 = v1 , k2 = v2 "); + + assertThat(map).containsExactly( + entry("k1 ", " v1"), + entry("k2 ", " v2") + ); + } + + @Test public void trimOWSAroundKeyValueSeparator() { + entrySplitter = EntrySplitter.newBuilder() + .trimOWSAroundEntrySeparator(false) + .trimOWSAroundKeyValueSeparator(true) + .shouldThrow(true).build(); + + entrySplitter.parse(parseIntoMap, map, " k1 = v1 , k2 = v2 "); + + assertThat(map).containsExactly( + entry(" k1", "v1 "), + entry(" k2", "v2 ") + ); + } + + @Test public void trimOWSAroundSeparators() { + entrySplitter = EntrySplitter.newBuilder() + .trimOWSAroundEntrySeparator(true) + .trimOWSAroundKeyValueSeparator(true) + .shouldThrow(true).build(); + + entrySplitter.parse(parseIntoMap, map, " k1 = v1 , k2 = v2 "); + + assertThat(map).containsExactly( + entry("k1", "v1"), + entry("k2", "v2") + ); + } + + @Test public void trimOWSAroundNothing() { + entrySplitter = EntrySplitter.newBuilder() + .trimOWSAroundEntrySeparator(false) + .trimOWSAroundKeyValueSeparator(false) + .shouldThrow(true).build(); + + entrySplitter.parse(parseIntoMap, map, " k1 = v1 , k2 = v2 "); + + assertThat(map).containsExactly( + entry(" k1 ", " v1 "), + entry(" k2 ", " v2 ") + ); + } + + @Test public void toleratesButIgnores_empty() { + entrySplitter.parse(parseIntoMap, map, ""); + + assertThat(map.isEmpty()); + } + + @Test public void toleratesButIgnores_onlyWhitespace() { + for (String w : Arrays.asList(" ", "\t")) { + entrySplitter.parse(parseIntoMap, map, w); + entrySplitter.parse(parseIntoMap, map, w + w); + } + + assertThat(map.isEmpty()); + } + + @Test public void toleratesButIgnores_emptyMembers() { + for (String w : Arrays.asList(" ", "\t")) { + entrySplitter.parse(parseIntoMap, map, ","); + entrySplitter.parse(parseIntoMap, map, w + ","); + entrySplitter.parse(parseIntoMap, map, "," + w); + entrySplitter.parse(parseIntoMap, map, ",,"); + entrySplitter.parse(parseIntoMap, map, "," + w + ","); + entrySplitter.parse(parseIntoMap, map, w + "," + w + "," + w); + } + + assertThat(map.isEmpty()); + } + + @Test public void builder_illegal() { + EntrySplitter.Builder builder = EntrySplitter.newBuilder(); + + assertThatThrownBy(() -> builder.maxEntries(-1)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("maxEntries <= 0"); + assertThatThrownBy(() -> builder.maxEntries(0)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("maxEntries <= 0"); + + assertThatThrownBy(() -> builder.entrySeparator((char) 0)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("entrySeparator == 0"); + + assertThatThrownBy(() -> builder.keyValueSeparator((char) 0)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("keyValueSeparator == 0"); + + builder.keyValueSeparator(';').entrySeparator(';'); + assertThatThrownBy(builder::build) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("entrySeparator == keyValueSeparator"); + } + + @Test public void parse_badParameters() { + assertThatThrownBy(() -> entrySplitter.parse(null, map, "")) + .isInstanceOf(NullPointerException.class) + .hasMessage("handler == null"); + assertThatThrownBy(() -> entrySplitter.parse(parseIntoMap, null, "")) + .isInstanceOf(NullPointerException.class) + .hasMessage("target == null"); + assertThatThrownBy(() -> entrySplitter.parse(parseIntoMap, map, null)) + .isInstanceOf(NullPointerException.class) + .hasMessage("input == null"); + + assertThatThrownBy(() -> entrySplitter.parse(parseIntoMap, map, "", -1, 1)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("beginIndex < 0"); + + assertThatThrownBy(() -> entrySplitter.parse(parseIntoMap, map, "", 0, 2)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("endIndex > input.length()"); + } +} diff --git a/brave/src/test/java/brave/internal/HexCodecTest.java b/brave/src/test/java/brave/internal/codec/HexCodecTest.java similarity index 94% rename from brave/src/test/java/brave/internal/HexCodecTest.java rename to brave/src/test/java/brave/internal/codec/HexCodecTest.java index caf3d78599..655ecc7b0c 100644 --- a/brave/src/test/java/brave/internal/HexCodecTest.java +++ b/brave/src/test/java/brave/internal/codec/HexCodecTest.java @@ -11,13 +11,13 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ -package brave.internal; +package brave.internal.codec; import org.junit.Test; -import static brave.internal.HexCodec.lenientLowerHexToUnsignedLong; -import static brave.internal.HexCodec.lowerHexToUnsignedLong; -import static brave.internal.HexCodec.toLowerHex; +import static brave.internal.codec.HexCodec.lenientLowerHexToUnsignedLong; +import static brave.internal.codec.HexCodec.lowerHexToUnsignedLong; +import static brave.internal.codec.HexCodec.toLowerHex; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown; diff --git a/brave/src/test/java/brave/internal/IpLiteralTest.java b/brave/src/test/java/brave/internal/codec/IpLiteralTest.java similarity index 97% rename from brave/src/test/java/brave/internal/IpLiteralTest.java rename to brave/src/test/java/brave/internal/codec/IpLiteralTest.java index c0d9e91a73..c5c9b437bd 100644 --- a/brave/src/test/java/brave/internal/IpLiteralTest.java +++ b/brave/src/test/java/brave/internal/codec/IpLiteralTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2019 The OpenZipkin Authors + * 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 @@ -11,7 +11,7 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ -package brave.internal; +package brave.internal.codec; import org.junit.Test; diff --git a/brave/src/test/java/brave/internal/JsonEscaperTest.java b/brave/src/test/java/brave/internal/codec/JsonEscaperTest.java similarity index 94% rename from brave/src/test/java/brave/internal/JsonEscaperTest.java rename to brave/src/test/java/brave/internal/codec/JsonEscaperTest.java index a434a9270b..bbc8223766 100644 --- a/brave/src/test/java/brave/internal/JsonEscaperTest.java +++ b/brave/src/test/java/brave/internal/codec/JsonEscaperTest.java @@ -11,11 +11,11 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ -package brave.internal; +package brave.internal.codec; import org.junit.Test; -import static brave.internal.JsonEscaper.jsonEscape; +import static brave.internal.codec.JsonEscaper.jsonEscape; import static org.assertj.core.api.Assertions.assertThat; // initially a copy of zipkin2.internal.JsonEscaperTest diff --git a/brave/src/test/java/brave/propagation/TraceContextTest.java b/brave/src/test/java/brave/propagation/TraceContextTest.java index 0207cb476f..63b06c0025 100644 --- a/brave/src/test/java/brave/propagation/TraceContextTest.java +++ b/brave/src/test/java/brave/propagation/TraceContextTest.java @@ -13,7 +13,7 @@ */ package brave.propagation; -import brave.internal.HexCodec; +import brave.internal.codec.HexCodec; import java.util.Arrays; import org.junit.Test; diff --git a/instrumentation/benchmarks/src/main/java/brave/baggage/BaggagePropagationBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/baggage/BaggagePropagationBenchmarks.java index e641ef4ca6..0cd0152476 100644 --- a/instrumentation/benchmarks/src/main/java/brave/baggage/BaggagePropagationBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/baggage/BaggagePropagationBenchmarks.java @@ -14,7 +14,7 @@ package brave.baggage; import brave.baggage.BaggagePropagationConfig.SingleBaggageField; -import brave.internal.HexCodec; +import brave.internal.codec.HexCodec; import brave.propagation.B3Propagation; import brave.propagation.Propagation; import brave.propagation.TraceContext; diff --git a/instrumentation/benchmarks/src/main/java/brave/grpc/GrpcPropagationBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/grpc/GrpcPropagationBenchmarks.java index 76d80d9210..3153d0e75e 100644 --- a/instrumentation/benchmarks/src/main/java/brave/grpc/GrpcPropagationBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/grpc/GrpcPropagationBenchmarks.java @@ -14,7 +14,7 @@ package brave.grpc; import brave.grpc.GrpcPropagation.TagsBin; -import brave.internal.HexCodec; +import brave.internal.codec.HexCodec; import brave.propagation.B3Propagation; import brave.propagation.Propagation; import brave.propagation.TraceContext; diff --git a/instrumentation/benchmarks/src/main/java/brave/grpc/TraceContextBinaryMarshallerBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/grpc/TraceContextBinaryMarshallerBenchmarks.java index 6718b67be2..8828cfd619 100644 --- a/instrumentation/benchmarks/src/main/java/brave/grpc/TraceContextBinaryMarshallerBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/grpc/TraceContextBinaryMarshallerBenchmarks.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2019 The OpenZipkin Authors + * 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 @@ -13,7 +13,7 @@ */ package brave.grpc; -import brave.internal.HexCodec; +import brave.internal.codec.HexCodec; import brave.propagation.TraceContext; import java.util.concurrent.TimeUnit; import org.openjdk.jmh.annotations.Benchmark; diff --git a/instrumentation/benchmarks/src/main/java/brave/propagation/B3PropagationBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/propagation/B3PropagationBenchmarks.java index 54f0016837..652076cb94 100644 --- a/instrumentation/benchmarks/src/main/java/brave/propagation/B3PropagationBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/propagation/B3PropagationBenchmarks.java @@ -13,7 +13,7 @@ */ package brave.propagation; -import brave.internal.HexCodec; +import brave.internal.codec.HexCodec; import brave.propagation.TraceContext.Extractor; import brave.propagation.TraceContext.Injector; import java.util.Collections; diff --git a/instrumentation/benchmarks/src/main/java/brave/propagation/B3SinglePropagationBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/propagation/B3SinglePropagationBenchmarks.java index 2d0bd5faa1..cd606014f3 100644 --- a/instrumentation/benchmarks/src/main/java/brave/propagation/B3SinglePropagationBenchmarks.java +++ b/instrumentation/benchmarks/src/main/java/brave/propagation/B3SinglePropagationBenchmarks.java @@ -13,7 +13,7 @@ */ package brave.propagation; -import brave.internal.HexCodec; +import brave.internal.codec.HexCodec; import brave.propagation.TraceContext.Extractor; import brave.propagation.TraceContext.Injector; import java.util.Collections; diff --git a/instrumentation/kafka-clients/src/test/java/brave/kafka/clients/ITKafkaTracing.java b/instrumentation/kafka-clients/src/test/java/brave/kafka/clients/ITKafkaTracing.java index 4cb2fd9c28..12a4800e7e 100644 --- a/instrumentation/kafka-clients/src/test/java/brave/kafka/clients/ITKafkaTracing.java +++ b/instrumentation/kafka-clients/src/test/java/brave/kafka/clients/ITKafkaTracing.java @@ -13,7 +13,7 @@ */ package brave.kafka.clients; -import brave.internal.HexCodec; +import brave.internal.codec.HexCodec; import brave.internal.propagation.StringPropagationAdapter; import brave.messaging.MessagingRuleSampler; import brave.messaging.MessagingTracing;