Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Returns invalid traceflags for a bad string parse instead of throwing… #2937

Closed
wants to merge 1 commit into from
Closed

Returns invalid traceflags for a bad string parse instead of throwing… #2937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/all/src/main/java/io/opentelemetry/api/trace/ImmutableSpanContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ static SpanContext create(
TraceFlags traceFlags,
TraceState traceState,
boolean remote) {
if (SpanId.isValid(spanIdHex) && TraceId.isValid(traceIdHex)) {
if (SpanId.isValid(spanIdHex) && TraceId.isValid(traceIdHex) && traceFlags.isValid()) {
return createInternal(
traceIdHex, spanIdHex, traceFlags, traceState, remote, /* valid= */ true);
}
Expand Down
14 changes: 12 additions & 2 deletions api/all/src/main/java/io/opentelemetry/api/trace/ImmutableTraceFlags.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
package io.opentelemetry.api.trace;

import io.opentelemetry.api.internal.OtelEncodingUtils;
import java.util.Objects;
import javax.annotation.concurrent.Immutable;

@Immutable
Expand All @@ -15,6 +14,7 @@ final class ImmutableTraceFlags implements TraceFlags {
// Bit to represent whether trace is sampled or not.
private static final byte SAMPLED_BIT = 0x01;

private static final ImmutableTraceFlags INVALID = new ImmutableTraceFlags((byte) 0);
static final ImmutableTraceFlags DEFAULT = fromByte((byte) 0x00);
static final ImmutableTraceFlags SAMPLED = fromByte(SAMPLED_BIT);
static final int HEX_LENGTH = 2;
Expand All @@ -24,7 +24,12 @@ final class ImmutableTraceFlags implements TraceFlags {

// Implementation of the TraceFlags.fromHex().
static ImmutableTraceFlags fromHex(CharSequence src, int srcOffset) {
Objects.requireNonNull(src, "src");
// TODO: Avoid calling `subSequence`.
if (src == null
|| src.length() < srcOffset + 2
|| !OtelEncodingUtils.isValidBase16String(src.subSequence(srcOffset, srcOffset + 2))) {
return INVALID;
}
return fromByte(
OtelEncodingUtils.byteFromBase16(src.charAt(srcOffset), src.charAt(srcOffset + 1)));
}
Expand Down Expand Up @@ -65,6 +70,11 @@ public byte asByte() {
return this.byteRep;
}

@Override
public boolean isValid() {
return this != INVALID;
}

@Override
public String toString() {
return asHex();
Expand Down
4 changes: 3 additions & 1 deletion api/all/src/main/java/io/opentelemetry/api/trace/SpanContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ default boolean isSampled() {
* @return {@code true} if this {@code SpanContext} is valid.
*/
default boolean isValid() {
return TraceId.isValid(getTraceId()) && SpanId.isValid(getSpanId());
return TraceId.isValid(getTraceId())
&& SpanId.isValid(getSpanId())
&& getTraceFlags().isValid();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a change from the specification, I believe, which says that the SpanContext is valid iff the spanid and traceid are valid, but doesn't mention trace flags.

}

/**
Expand Down
11 changes: 8 additions & 3 deletions api/all/src/main/java/io/opentelemetry/api/trace/TraceFlags.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,12 @@ static TraceFlags getSampled() {

/**
* Returns the {@link TraceFlags} converted from the given lowercase hex (base16) representation.
* If the input cannot be parsed, a {@link TraceFlags} will be returned where {@link
* TraceFlags#isValid()} returns {@code false}.
*
* @param src the buffer where the hex (base16) representation of the {@link TraceFlags} is.
* @param srcOffset the offset int buffer.
* @return the {@link TraceFlags} converted from the given lowercase hex (base16) representation.
* @throws NullPointerException if {@code src} is null.
* @throws IndexOutOfBoundsException if {@code src} is too short.
* @throws IllegalArgumentException if invalid characters in the {@code src}.
*/
static TraceFlags fromHex(CharSequence src, int srcOffset) {
return ImmutableTraceFlags.fromHex(src, srcOffset);
Expand Down Expand Up @@ -90,4 +89,10 @@ static TraceFlags fromByte(byte traceFlagsByte) {
* @return the byte representation of the {@link TraceFlags}.
*/
byte asByte();

/**
* Returns whether this {@link TraceFlags} is valid. An invalid {@link TraceFlags} should
* generally be discarded.
*/
boolean isValid();
}
29 changes: 11 additions & 18 deletions api/all/src/main/java/io/opentelemetry/api/trace/propagation/W3CTraceContextPropagator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,26 +207,19 @@ private static SpanContext extractContextFromTraceParent(String traceparent) {
return SpanContext.getInvalid();
}

try {
String version = traceparent.substring(0, 2);
if (!VALID_VERSIONS.contains(version)) {
return SpanContext.getInvalid();
}
if (version.equals(VERSION_00) && traceparent.length() > TRACEPARENT_HEADER_SIZE) {
return SpanContext.getInvalid();
}

String traceId =
traceparent.substring(TRACE_ID_OFFSET, TRACE_ID_OFFSET + TraceId.getLength());
String spanId = traceparent.substring(SPAN_ID_OFFSET, SPAN_ID_OFFSET + SpanId.getLength());

TraceFlags traceFlags = TraceFlags.fromHex(traceparent, TRACE_OPTION_OFFSET);
return SpanContext.createFromRemoteParent(
traceId, spanId, traceFlags, TraceState.getDefault());
} catch (IllegalArgumentException e) {
logger.fine("Unparseable traceparent header. Returning INVALID span context.");
String version = traceparent.substring(0, 2);
if (!VALID_VERSIONS.contains(version)) {
return SpanContext.getInvalid();
}
if (version.equals(VERSION_00) && traceparent.length() > TRACEPARENT_HEADER_SIZE) {
return SpanContext.getInvalid();
}

String traceId = traceparent.substring(TRACE_ID_OFFSET, TRACE_ID_OFFSET + TraceId.getLength());
String spanId = traceparent.substring(SPAN_ID_OFFSET, SPAN_ID_OFFSET + SpanId.getLength());

TraceFlags traceFlags = TraceFlags.fromHex(traceparent, TRACE_OPTION_OFFSET);
return SpanContext.createFromRemoteParent(traceId, spanId, traceFlags, TraceState.getDefault());
}

private static TraceState extractTraceState(String traceStateHeader) {
Expand Down
8 changes: 8 additions & 0 deletions api/all/src/test/java/io/opentelemetry/api/trace/SpanContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ void isValid() {
.isFalse();
assertThat(first.isValid()).isTrue();
assertThat(second.isValid()).isTrue();
assertThat(
SpanContext.create(
FIRST_TRACE_ID,
FIRST_SPAN_ID,
TraceFlags.fromHex("abc", 3),
TraceState.getDefault())
.isValid())
.isFalse();
}

@Test
Expand Down
7 changes: 7 additions & 0 deletions api/all/src/test/java/io/opentelemetry/api/trace/TraceFlagsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ void toFromHex() {
}
}

@Test
void toFromHex_Invalid() {
assertThat(TraceFlags.fromHex(null, 0).isValid()).isFalse();
assertThat(TraceFlags.fromHex("hex", 0).isValid()).isFalse();
assertThat(TraceFlags.fromHex("aa", 1).isValid()).isFalse();
}

@Test
void toFromByte() {
for (int i = 0; i < 256; i++) {
Expand Down