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.
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
fix(logging): allow X-Cloud-Trace-Context fields to be optional #3062
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this change @nicoleczhu! I've added some suggestions, please take a look.
logging/logging.go
Outdated
if len(sub) != 4 { | ||
return | ||
} | ||
traceID, spanID, traceSampled = matches[1], matches[3], (matches[5] == "1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please restore (and adjust) the length checks to ensure that we have at least 5 components otherwise we'll encounter crashes later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked this one, given the new regex pattern/FindStringSubmatch, it will always return 6 components even if string s is "" or very wrongly formatted. See sample code here: https://play.golang.org/p/-adjLHwv1VD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That regex isn't very legible, I'd say just for the sake of maintaining invariants and easily inspectable behavior, let's enforce those length counts. A single change to that regex without the appropriate checks can cause later surprises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. How about I add descriptive comments to the regex itself, so folks changing the regex don't encounter surprises.
I'd really prefer disclosing in comments, rather than enforcing a if(true)
type of check in code. I think checking length might also confuse folks looking at the regex in the future, in that it suggests the regex pattern could return various lengths (when it never will).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Judgement call/preference :) but personally I'd ensure that count and even panic if not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update:
- I broke apart the regex, adding comments for readability.
- I used non-capturing groups this time, so the matched indeces are more intuitive (1,2,3) rather than (1,3,5).
Thanks for the style feedback on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes: #1590
Changes according to spec:
trace
,spanID
andtraceSampled
fields in X-Cloud-Trace-Context can be optional (see context/trace_context.proto)Details:
Intended Use case
The following header specifications by users are now recognized: