fix(logging): Fixed input validation for X-Cloud-Trace-Context; encoded spanID from XCTC header into hex string. #10979
Conversation
from XCTC header into hex string.
codyoss
changed the title
product-auto-label
bot
added
the
api: logging
gkevinzheng
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
minimize risk of breaking change
michaelsafyan
left a comment
michaelsafyan
left a comment
There was a problem hiding this comment.
Thanks for taking care of this! Per your request over Google Chat, I've left some feedback on this PR. I will defer, however, to your judgement and that of the other reviewer concerning what subset of this feedback to act on. Thanks!
| // "X-Cloud-Trace-Context: 105445aa7843bc8bf206b120001000/1;o=1" | ||
| // "X-Cloud-Trace-Context: 105445aa7843bc8bf206b12000100000/1;o=1" | ||
| // | ||
| // We expect: |
There was a problem hiding this comment.
It might be easier to understand if you phrase the comment as:
// Parameters:
// s - ...
//
// Returns:
// ...
With respect to "We expect", it is unclear whether this is referring to the input or the output.
If it is intended to describe the input, then shouldn't the documentation for spanID say it is a decimal string?
| spanID = fmt.Sprintf("%016x", intSpanID) | ||
| } | ||
| } | ||
| return |
There was a problem hiding this comment.
Optional readability suggestion: consider returning the values explicitly rather than relying on named return values. It was a little confusing seeing this return without the list of variables that are returned.
logging/logging.go
Outdated
| } | ||
|
|
||
| if spanID == "0" { | ||
| spanID = "" |
There was a problem hiding this comment.
You may want to consider similar validation/conversion on the trace ID (to ensure that it isn't all-zero and to ensure that it is zero-padded if smaller than the W3C spec length).
logging/logging.go
Outdated
| // * traceID (optional): "105445aa7843bc8bf206b120001000" | ||
| // * spanID (optional): "1" | ||
| // * traceSampled (optional): true | ||
| // * traceID (optional, 32-bit hex string): "105445aa7843bc8bf206b12000100000" |
There was a problem hiding this comment.
The trace ID is 128-bit, not 32-bit; it is represented as a 16-byte (32-char) array. This comment and the next one may be confusing the number of symbols in the string with the number of bits in the numeric value.
| TraceSampled: true, | ||
| }, | ||
| }, { | ||
| name: "X-Trace-Context with Span ID too large", |
There was a problem hiding this comment.
Is there any logging or other debug information that can be output in such a case?
| // Convert to 16 byte unsigned hex string | ||
| intSpanID, err := strconv.ParseUint(spanID, 10, 64) | ||
| if err != nil || intSpanID == 0 { | ||
| spanID = "" |
There was a problem hiding this comment.
Is it possible to leave some clues when this happens?
logging/logging.go
Outdated
| } | ||
|
|
||
| if spanID == "0" { | ||
| spanID = "" |
There was a problem hiding this comment.
Is it possible to leave some clues when this happens?
4 participants
Changes:
X-Cloud-Trace-Contexthas moved, so that comment has been updated.spanIDin the return value ofdeconstructXCloudTraceContextinto 16-bit hexadecimal string instead of decimal string.Fixes #10910