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

Validate the span id and trace id when creating the SpanContext #2728

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Validate the span id and trace id when creating the SpanContext #2728

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
15 changes: 12 additions & 3 deletions 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 @@ -14,21 +14,30 @@
abstract class ImmutableSpanContext implements SpanContext {

static final SpanContext INVALID =
create(
createInternal(
TraceId.getInvalid(),
SpanId.getInvalid(),
TraceFlags.getDefault(),
TraceState.getDefault(),
/* remote= */ false);

private static AutoValue_ImmutableSpanContext createInternal(
String traceId, String spanId, TraceFlags traceFlags, TraceState traceState, boolean remote) {
return new AutoValue_ImmutableSpanContext(
traceId, spanId, traceFlags, traceState, /* remote$= */ remote);
Comment on lines +26 to +27
Copy link
Member

Choose a reason for hiding this comment

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

Now that you calculate the isValid it can be a property in the AutoValue so no need to recalculate that or to use memoization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

excellent idea. let's create an issue to track that optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2752

Copy link
Member

Choose a reason for hiding this comment

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

Consider to remove the comment /* remote$= */ not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it always baffles me when errorprone will complain about this, and when it won't. I'm sure I added it because errorprone complained, but there might have been some refactoring after that.

}

static SpanContext create(
String traceIdHex,
String spanIdHex,
TraceFlags traceFlags,
TraceState traceState,
boolean remote) {
return new AutoValue_ImmutableSpanContext(
traceIdHex, spanIdHex, traceFlags, traceState, remote);
if (SpanId.isValid(spanIdHex) && TraceId.isValid(traceIdHex)) {
return createInternal(traceIdHex, spanIdHex, traceFlags, traceState, remote);
}
return createInternal(
Copy link
Member

Choose a reason for hiding this comment

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

Should we return the INVALID SC?

Copy link
Member

Choose a reason for hiding this comment

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

The reason is that it may seem strange to propagate flags and state but not one of the IDs if the other one is invalid

Copy link
Member

Choose a reason for hiding this comment

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

So we either drop everything or we keep everything that is valid. I think the current implementation is in the middle and not ideal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were tests depending on this behavior. I'll need to figure out what they were trying to accomplish.

Copy link
Member

Choose a reason for hiding this comment

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

I think I know why. There are 2 cases:

  1. is invalid hex string - in this case I think we should return SpanContext.getInvalid()
  2. is equal with TraceIs.getInvalid() - this is used in tests I guess, probably we should preserve all values.

Not sure if that is the behavior we want, but this is a guess for the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes...this will take a bit of research that we can create a follow-up issue to deal with, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added #2751

TraceId.getInvalid(), SpanId.getInvalid(), traceFlags, traceState, remote);
}

@Override
Expand Down
10 changes: 4 additions & 6 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 @@ -214,12 +214,10 @@ private static SpanContext extractContextFromTraceParent(String traceparent) {
String traceId =
traceparent.substring(TRACE_ID_OFFSET, TRACE_ID_OFFSET + TraceId.getLength());
String spanId = traceparent.substring(SPAN_ID_OFFSET, SPAN_ID_OFFSET + SpanId.getLength());
if (TraceId.isValid(traceId) && SpanId.isValid(spanId)) {
TraceFlags traceFlags = TraceFlags.fromHex(traceparent, TRACE_OPTION_OFFSET);
return SpanContext.createFromRemoteParent(
traceId, spanId, traceFlags, TraceState.getDefault());
}
return SpanContext.getInvalid();

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.");
return SpanContext.getInvalid();
Expand Down
20 changes: 0 additions & 20 deletions ...agators/src/main/java/io/opentelemetry/extension/trace/propagation/AwsXrayPropagator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,26 +171,6 @@ private static <C> SpanContext getSpanContextFromHeader(C carrier, Getter<C> get
}
// TODO: Put the arbitrary TraceHeader keys in OT trace state
}
if (!TraceId.isValid(traceId)) {
logger.fine(
"Invalid TraceId in X-Ray trace header: '"
+ TRACE_HEADER_KEY
+ "' with value "
+ traceHeader
+ "'. Returning INVALID span context.");
return SpanContext.getInvalid();
}

if (!SpanId.isValid(spanId)) {
logger.fine(
"Invalid ParentId in X-Ray trace header: '"
+ TRACE_HEADER_KEY
+ "' with value "
+ traceHeader
+ "'. Returning INVALID span context.");
return SpanContext.getInvalid();
}

if (isSampled == null) {
logger.fine(
"Invalid Sampling flag in X-Ray trace header: '"
Expand Down
3 changes: 0 additions & 3 deletions ...pagators/src/main/java/io/opentelemetry/extension/trace/propagation/JaegerPropagator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,6 @@ private static SpanContext buildSpanContext(String traceId, String spanId, Strin
try {
String otelTraceId = StringUtils.padLeft(traceId, MAX_TRACE_ID_LENGTH);
String otelSpanId = StringUtils.padLeft(spanId, MAX_SPAN_ID_LENGTH);
if (!TraceId.isValid(otelTraceId) || !SpanId.isValid(otelSpanId)) {
return SpanContext.getInvalid();
}
int flagsInt = Integer.parseInt(flags);
return SpanContext.createFromRemoteParent(
otelTraceId,
Expand Down