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.

Sign up for GitHub

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

Jump to bottom

fix(logging): allow X-Cloud-Trace-Context fields to be optional #3062

Merged
merged 7 commits into from Oct 22, 2020
Merged

fix(logging): allow X-Cloud-Trace-Context fields to be optional #3062

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
46a3c68
docs(logging): update doc link explaining x-cloud-trace-context
Oct 20, 2020
0a795ea
fix(logging): X-Cloud-Trace-Context fields can be optional (traceSamp…
Oct 21, 2020
1edfd6f
style: go fmt logging.go
Oct 21, 2020
29c2299
style(logging): updated inline comments to be clearer
Oct 21, 2020
c1ddf04
Merge branch 'master' into fix1590
0xSage Oct 22, 2020
34762a1
refactor(logging): reCloudTraceContext regex is more readable
Oct 22, 2020
eed8ecb
Merge branch 'master' into fix1590
0xSage Oct 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 8 additions & 16 deletions logging/logging.go
View file
Open in desktop
Expand Up @@ -873,33 +873,25 @@ func (l *Logger) writeLogEntries(entries []*logpb.LogEntry) {
// (for example by calling SetFlags or SetPrefix).
func (l *Logger) StandardLogger(s Severity) *log.Logger { return l.stdLoggers[s] }

var reCloudTraceContext = regexp.MustCompile(`([a-f\d]+)/([a-f\d]+);o=(\d)`)
var reCloudTraceContext = regexp.MustCompile(`([a-f\d]+)?(/([a-f\d]+))?(;o=(\d))?`)

func deconstructXCloudTraceContext(s string) (traceID, spanID string, traceSampled bool) {
// As per the format described at https://cloud.google.com/trace/docs/troubleshooting#force-trace
// As per the format described at https://cloud.google.com/trace/docs/setup#force-trace
// "X-Cloud-Trace-Context: TRACE_ID/SPAN_ID;o=TRACE_TRUE"
// for example:
// "X-Cloud-Trace-Context: 105445aa7843bc8bf206b120001000/0;o=1"
// "X-Cloud-Trace-Context: 105445aa7843bc8bf206b120001000/1;o=1"
//
// We expect:
// * traceID: "105445aa7843bc8bf206b120001000"
// * spanID: ""
// * traceSampled: true
matches := reCloudTraceContext.FindAllStringSubmatch(s, -1)
if len(matches) != 1 {
return
}
// * traceID (optional): "105445aa7843bc8bf206b120001000"
// * spanID (optional): "1"
// * traceSampled (optional): true
matches := reCloudTraceContext.FindStringSubmatch(s)

sub := matches[0]
if len(sub) != 4 {
return
}
traceID, spanID, traceSampled = matches[1], matches[3], (matches[5] == "1")
Copy link
Contributor

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 •
edited

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

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

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

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

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!

0xSage marked this conversation as resolved.
Show resolved Hide resolved

traceID, spanID = sub[1], sub[2]
if spanID == "0" {
spanID = ""
}
traceSampled = sub[3] == "1"

return
}
Expand Down
38 changes: 37 additions & 1 deletion logging/logging_unexported_test.go
View file
Open in desktop
Expand Up @@ -288,18 +288,54 @@ func TestToLogEntryTrace(t *testing.T) {
},
logging.LogEntry{Trace: "projects/P/traces/105445aa7843bc8bf206b120001000", SpanId: "000000000000004a", TraceSampled: true},
},
{
"X-Trace-Context header with blank trace",
Entry{
HTTPRequest: &HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"/0;o=1"}},
},
},
},
logging.LogEntry{TraceSampled: true},
},
{
"X-Trace-Context header with blank span",
Entry{
HTTPRequest: &HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/0;o=0"}},
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/;o=0"}},
},
},
},
logging.LogEntry{Trace: "projects/P/traces/105445aa7843bc8bf206b120001000"},
},
{
"X-Trace-Context header with missing traceSampled aka ?o=*",
Entry{
HTTPRequest: &HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {"105445aa7843bc8bf206b120001000/0"}},
},
},
},
logging.LogEntry{Trace: "projects/P/traces/105445aa7843bc8bf206b120001000"},
},
{
"X-Trace-Context header with all blank fields",
Entry{
HTTPRequest: &HTTPRequest{
Request: &http.Request{
URL: u,
Header: http.Header{"X-Cloud-Trace-Context": {""}},
},
},
},
logging.LogEntry{},
},
{
"Invalid X-Trace-Context header but already set TraceID",
Entry{
Expand Down