Validate the span id and trace id when creating the SpanContext #2728
Conversation
And, remove that extra validation from propagators.
jkwatson
requested review from
anuraaga,
arminru,
bogdandrutu,
carlosalberto,
Oberon00,
pavolloffay,
thisthat and
tylerbenson
as code owners
|
|
| if (SpanId.isValid(spanIdHex) && TraceId.isValid(traceIdHex)) { | ||
| return createInternal(traceIdHex, spanIdHex, traceFlags, traceState, remote); | ||
| } | ||
| return createInternal( |
There was a problem hiding this comment.
Should we return the INVALID SC?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
So we either drop everything or we keep everything that is valid. I think the current implementation is in the middle and not ideal
There was a problem hiding this comment.
There were tests depending on this behavior. I'll need to figure out what they were trying to accomplish.
There was a problem hiding this comment.
I think I know why. There are 2 cases:
- is invalid hex string - in this case I think we should return SpanContext.getInvalid()
- 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.
There was a problem hiding this comment.
yes...this will take a bit of research that we can create a follow-up issue to deal with, I think.
| return new AutoValue_ImmutableSpanContext( | ||
| traceId, spanId, traceFlags, traceState, /* remote$= */ remote); |
There was a problem hiding this comment.
Now that you calculate the isValid it can be a property in the AutoValue so no need to recalculate that or to use memoization.
There was a problem hiding this comment.
excellent idea. let's create an issue to track that optimization.
| 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); |
There was a problem hiding this comment.
Consider to remove the comment /* remote$= */ not needed
There was a problem hiding this comment.
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.
|
Added issues to track the optimization and investigation of the slightly strange half-way-valid usage. |
And, remove that extra validation from propagators.
I didn't remove it from the B3 Propagator(s) yet... that will take a bit more work to untangle.
Resolves #2719