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

cli,log: allow use of debug merge-logs on older logs #68282

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Jul 30, 2021

Fixes #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.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Yes this looks good. What does this miss?

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @cameronnunez)

@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 5, 2021

Yes this looks good. What does this miss?

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.

@cameronnunez cameronnunez added A-logging In and around the logging infrastructure. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-server-and-security DB Server & Security labels Sep 2, 2021
@cameronnunez cameronnunez added this to In progress in DB Server & Security via automation Sep 2, 2021
@cameronnunez cameronnunez force-pushed the ajwerner/explicitly-decode-format-in-debug-merge-logs branch 3 times, most recently from 43c7032 to 9fde06d Compare September 7, 2021 23:37
@cameronnunez cameronnunez marked this pull request as ready for review September 7, 2021 23:38
@cameronnunez cameronnunez requested review from a team as code owners September 7, 2021 23:38
@cameronnunez
Copy link
Contributor

cameronnunez commented Sep 7, 2021

Added the necessary testing and heuristics for fallback. PTAL @ajwerner @knz @aliher1911

@cameronnunez cameronnunez removed request for a team and cameronnunez September 7, 2021 23:39
@cameronnunez cameronnunez changed the title [DNM] cli,logs: export method to decode format, use in merge-logs cli,logs: export method to decode format, use in merge-logs Sep 7, 2021
@cameronnunez cameronnunez changed the title cli,logs: export method to decode format, use in merge-logs cli,log: export method to decode format, use in merge-logs Sep 7, 2021
@cameronnunez cameronnunez force-pushed the ajwerner/explicitly-decode-format-in-debug-merge-logs branch from 99ff240 to 36e94c3 Compare September 7, 2021 23:59
@cameronnunez cameronnunez changed the title cli,log: export method to decode format, use in merge-logs cli,log: allow use of debug merge-logs on older logs Sep 8, 2021
@cameronnunez cameronnunez force-pushed the ajwerner/explicitly-decode-format-in-debug-merge-logs branch 2 times, most recently from fcb454d to 89f9864 Compare September 9, 2021 20:14
@ajwerner ajwerner requested a review from a team as a code owner September 9, 2021 20:14
@cameronnunez cameronnunez force-pushed the ajwerner/explicitly-decode-format-in-debug-merge-logs branch from 89f9864 to 6d1becd Compare September 9, 2021 20:15
@cameronnunez cameronnunez requested review from a team and removed request for a team September 9, 2021 20:31
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

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: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @aliher1911)


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.

@cameronnunez cameronnunez force-pushed the ajwerner/explicitly-decode-format-in-debug-merge-logs branch 2 times, most recently from 9a81527 to 2b3c08e Compare September 13, 2021 19:37
@cameronnunez cameronnunez changed the title cli,log: allow use of debug merge-logs on older logs [DNM] cli,log: allow use of debug merge-logs on older logs Sep 13, 2021
@cameronnunez cameronnunez force-pushed the ajwerner/explicitly-decode-format-in-debug-merge-logs branch 3 times, most recently from c199e3a to c4c3a19 Compare September 13, 2021 21:59
Copy link
Contributor

@cameronnunez cameronnunez left a comment

Choose a reason for hiding this comment

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

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: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @knz)


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 format entry 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.

@cameronnunez cameronnunez force-pushed the ajwerner/explicitly-decode-format-in-debug-merge-logs branch from c4c3a19 to 0fcc869 Compare September 14, 2021 21:59
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

:lgtm_strong: thank you!

Reviewed 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @aliher1911)

@knz
Copy link
Contributor

knz commented Sep 15, 2021

don't forget to retitle the PR before this merges

@cameronnunez cameronnunez force-pushed the ajwerner/explicitly-decode-format-in-debug-merge-logs branch from 0fcc869 to fd7d31e Compare September 15, 2021 19:43
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>
@cameronnunez cameronnunez force-pushed the ajwerner/explicitly-decode-format-in-debug-merge-logs branch from fd7d31e to 68ef2d0 Compare September 15, 2021 19:46
@cameronnunez cameronnunez changed the title [DNM] cli,log: allow use of debug merge-logs on older logs cli,log: allow use of debug merge-logs on older logs Sep 15, 2021
@cameronnunez
Copy link
Contributor

thanks for your review Raphael!

bors r=knz

@craig craig bot merged commit f278e64 into cockroachdb:master Sep 15, 2021
@craig
Copy link
Contributor

craig bot commented Sep 15, 2021

Build succeeded:

DB Server & Security automation moved this from In progress to Done 21.2 Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-logging In and around the logging infrastructure. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-server-and-security DB Server & Security
Projects
DB Server & Security
  
Done 21.2
Development

Successfully merging this pull request may close these issues.

Can't use debug merge-logs on logs from 20.2 unless explicitly provide flag
4 participants