Skip to content
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

Merged
merged 7 commits into from Oct 22, 2020
Merged

Conversation

@0xSage
Copy link
Contributor

@0xSage 0xSage commented Oct 21, 2020

Fixes: #1590

Changes according to spec:

  1. trace, spanID and traceSampled fields in X-Cloud-Trace-Context can be optional (see context/trace_context.proto)
  2. TraceSample should default to false (verified with nwang@)
  3. Trace/span IDs should still be included in log contexts even when traceSampled is false, because "A non-sampled trace value is still useful as a request correlation identifier." (LogEntry spec)

Details:
Intended Use case

The following header specifications by users are now recognized:
Screen Shot 2020-10-21 at 12 30 23 PM

@0xSage 0xSage requested review from codyoss and simonz130 Oct 21, 2020
@0xSage 0xSage self-assigned this Oct 21, 2020
@0xSage 0xSage requested a review from daniel-sanche Oct 21, 2020
Copy link
Contributor

@odeke-em odeke-em left a comment

Thank you for this change @nicoleczhu! I've added some suggestions, please take a look.

logging/logging.go Outdated Show resolved Hide resolved
logging/logging.go Outdated Show resolved Hide resolved
if len(sub) != 4 {
return
}
traceID, spanID, traceSampled = matches[1], matches[3], (matches[5] == "1")
Copy link
Contributor

@odeke-em odeke-em Oct 21, 2020

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.

Copy link
Contributor Author

@0xSage 0xSage Oct 21, 2020

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

Copy link
Contributor

@odeke-em odeke-em Oct 21, 2020

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.

Copy link
Contributor Author

@0xSage 0xSage Oct 21, 2020

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).

Copy link
Contributor

@odeke-em odeke-em Oct 21, 2020

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

Copy link
Contributor Author

@0xSage 0xSage Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update:

  1. I broke apart the regex, adding comments for readability.
  2. 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!

logging/logging.go Outdated Show resolved Hide resolved
logging/logging_unexported_test.go Outdated Show resolved Hide resolved
Copy link
Member

@codyoss codyoss left a comment

LGTM

@0xSage 0xSage merged commit 7ff03cf into master Oct 22, 2020
3 checks passed
@0xSage 0xSage deleted the fix1590 branch Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants