Add support for 16 hex character traceIds. #18
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtth
reviewed
Jun 28, 2021
src/Monitor/Tracing/Zipkin.hs
Outdated
Comment on lines
268
to
269
| decodeZipkinTraceID txt | T.length txt == 16 = decodeTraceID $ "0000000000000000" <> txt | ||
| | otherwise = decodeTraceID txt |
There was a problem hiding this comment.
Let's keep the indentation style consistent style and consolidate the common parts:
decodeZipkinTraceID txt =
let normalized = if T.length txt == 16 then "0000000000000000" <> txt else txt
in decodeTraceID normalizedSimilar below for indentation.
src/Monitor/Tracing/Zipkin.hs
Outdated
| -- | Takes into account that the first position of the b3 value is the 32 or 16 lower-hex character TraceId. | ||
| encodeZipkinTraceID :: TraceID -> Text | ||
| encodeZipkinTraceID traceId = let txt = encodeTraceID traceId | ||
| in fromMaybe txt $ T.stripPrefix "0000000000000000" txt |
There was a problem hiding this comment.
Let's consolidate the prefix constant, for ex as shortIDPrefix.
src/Monitor/Tracing/Zipkin.hs
Outdated
| optional = encodeSpanID <$> maybeToList mbParentID | ||
| in BS.intercalate "-" $ fmap T.encodeUtf8 $ required ++ optional | ||
|
|
||
| -- | Takes into account that the first position of the b3 value is the 32 or 16 lower-hex character TraceId. |
There was a problem hiding this comment.
Can you start the new doc comments with a quick blurb describing what the function does (before the specifics sentence here)? Something like "Decodes a trace ID from its Zipkin header representation, returning nothing if it is invalid. ...".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #12
Intentionally I only changed Zipkin.hs and not Trace/Internal.hs:
a) It's part of the Zipkin specification, so I thought it naturally belongs to Zipkin.hs 😄
b) Other propagation standards (e.g. https://www.w3.org/TR/trace-context/#trace-id) use 128-bit - so I believe it's okay to always use 128-bit internally
c) Even if this is a internal module - it's public API - so to only change things in Zipkin.hs reduces the chance of a breaking change
But it has the drawback that the handling is somehow distributed and the implementation looks "patched".
As always, just let me know what you think.