cli,log: allow use of debug merge-logs on older logs#68282
Conversation
|
This change is |
knz
left a comment
There was a problem hiding this comment.
Yes this looks good. What does this miss?
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @cameronnunez)
Testing and better heuristics for the fallback. For whatever reason, all of the input data for our merge logs tests do not carry a header. To validate this, we should add some test data involving files with some headers to show that this works. If nobody else gets to it, I'll get around to it when I find a gap. |
43c7032 to
9fde06d
Compare
|
Added the necessary testing and heuristics for fallback. PTAL @ajwerner @knz @aliher1911 |
99ff240 to
36e94c3
Compare
debug merge-logs on older logs
fcb454d to
89f9864
Compare
89f9864 to
6d1becd
Compare
knz
left a comment
There was a problem hiding this comment.
You need to adjust Example_format_error.
You'll probably need a new test input for it which does not use a [config] marker.
Reviewed 5 of 5 files at r2.
Reviewable status:
pkg/util/log/log_decoder.go, line 119 at r3 (raw file):
// If the absence of a counter in an entry is not represented by two ASCII spaces, the format must be crdb-v1. // If detected, determine that the log is crdb-v1 formatted. if re = regexp.MustCompile(
please extract MustCompile calls in the global scope so they get compiled just once.
pkg/util/log/log_decoder.go, line 121 at r3 (raw file):
if re = regexp.MustCompile( `(?m)^` + /* crdb-v1 Indicator */ `(?:.*\[config\] [a-zA-Z].*)$`,
This regexp is too flexible, I recommend you add the timestamp prefix, otherwise this will match a JSON entry which had the log format entry removed.
pkg/util/log/log_decoder.go, line 131 at r3 (raw file):
if re = regexp.MustCompile( `(?m)^` + /* crdb-v2 Indicator */ `(?:.*\[config\] [a-zA-Z].*)$`,
Ditto, add the timestmap prefix here too.
9a81527 to
2b3c08e
Compare
debug merge-logs on older logsdebug merge-logs on older logs
c199e3a to
c4c3a19
Compare
There was a problem hiding this comment.
Realized that I needed to change the test input entirely for Example_format_error since the cases of format error are reduced because of this PR. I substituted in a json formatted header there since that cannot be parsed yet.
Reviewable status:
pkg/util/log/log_decoder.go, line 119 at r3 (raw file):
Previously, knz (kena) wrote…
please extract MustCompile calls in the global scope so they get compiled just once.
Done.
pkg/util/log/log_decoder.go, line 121 at r3 (raw file):
Previously, knz (kena) wrote…
This regexp is too flexible, I recommend you add the timestamp prefix, otherwise this will match a JSON entry which had the
log formatentry removed.
I decided to change the regexp to match the line format entry. Let me know what you think about it.
pkg/util/log/log_decoder.go, line 131 at r3 (raw file):
Previously, knz (kena) wrote…
Ditto, add the timestmap prefix here too.
Same as above.
c4c3a19 to
0fcc869
Compare
knz
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status:
|
don't forget to retitle the PR before this merges |
0fcc869 to
fd7d31e
Compare
Fixes cockroachdb#68278. Log parsers require the flag --format when parsing older logs (because they do not contain format specification). With this patch, this is no longer a requirement as the log format is now inferred based on the structure of the log if no log format specification exists. Release justification: bug fix Release note (bug fix): The debug merge-logs command no longer returns an error when the log decoder attempts to parse older logs. Co-authored-by: Cameron Nunez <cameron@cockroachlabs.com>
fd7d31e to
68ef2d0
Compare
debug merge-logs on older logsdebug merge-logs on older logs
|
thanks for your review Raphael! bors r=knz |
|
Build succeeded: |
Fixes #68278.
Log parsers require the flag
--formatwhen parsing older logs (becausethey do not contain format specification). With this patch, this is no longer
a requirement as the log format is now inferred based on the structure of
the log if no log format specification exists.
Release justification: bug fix
Release note (bug fix): The debug merge-logs command no longer returns an error
when the log decoder attempts to parse older logs.