Make parsing of Google's X-Cloud-Trace-Context less strict#850
Conversation
The `;o=...` part is supposed to be optional
|
/cc @janstenpickle @catostrophe for better visibility. I'd appreciate your review on this PR 🙇 |
|
CCing also @TimWSpence 🙏 |
modules/kernel/src/main/scala/trace4cats/kernel/headers/GoogleCloudTraceToHeaders.scala
Outdated
Show resolved
Hide resolved
modules/kernel/src/main/scala/trace4cats/kernel/headers/GoogleCloudTraceToHeaders.scala
Show resolved
Hide resolved
| val headerPattern = | ||
| """(?xi) | ||
| |([0-9a-f]+) # trace ID | ||
| |([^\/]+) # trace ID |
There was a problem hiding this comment.
What motivates this change and the one below? Can the trace ID ever been non-hexadecimal or the span ID non-decimal?
There was a problem hiding this comment.
You will get error in either case. But this way you get more precise error. The regex should try to match only the rough format of the string.
janstenpickle
left a comment
janstenpickle
left a comment
There was a problem hiding this comment.
Thanks for this @sideeffffect, just had one nitpick on parsing the BigInt error handling, other than that happy to merge!
| spanId <- Either.fromOption( | ||
| SpanId.fromHexString("%016x".format(BigInt(spanId))), | ||
| Either.catchNonFatal(BigInt(spanId)).toOption.flatMap(bi => SpanId.fromHexString("%016x".format(bi))), | ||
| new Exception("invalid span ID") |
There was a problem hiding this comment.
The parse method supports surfacing an exception on the left of an either so I'd suggest doing this instead:
Either.catchNonFatal(BigInt(spanId)).flatMap(bi => Either.fromOption(SpanId.fromHexString("%016x".format(bi)), new Exception("invalid span ID")))
janstenpickle
left a comment
janstenpickle
left a comment
There was a problem hiding this comment.
LGTM, thanks @sideeffffect!
5 participants
The
;o=...part is supposed to be optional