util/log: extend logpb.Entry with more fine grained fields
#66096
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
cameronnunez
suggested changes
Jun 4, 2021
pkg/util/log/format_crdb_v1_test.go
Outdated
| "github.com/kr/pretty" | ||
| ) | ||
|
|
||
| func TestCrdbV1FormatDecode(t *testing.T) { |
There was a problem hiding this comment.
Suggested change
| func TestCrdbV1FormatDecode(t *testing.T) { | |
| func TestFormatCrdbV1Decode(t *testing.T) { |
For consistency, I suggest a slight change to the function name here.
|
|
||
| // Verify the truncation logic for reading logs that are longer than the | ||
| // default scanner can handle. | ||
| preambleLength := len(formatEntry(severity.INFO, channel.DEV, t1, 0, "clog_test.go", 136, ``, "")) |
There was a problem hiding this comment.
Each instance of the string "clog_test.go" in format_crdb_v1_test.go and testdata/crdb_v1 should be changed to the string "format_crdb_v1_test.go"
knz
commented
Jun 5, 2021
There was a problem hiding this comment.
nit: if you are only requesting minor changes, then it's better to leave your review disposition as "neutral" instead of "changes requested".
With "changes requested" I have to wait until you complete the review before I can merge the PR.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @cameronnunez)
pkg/util/log/format_crdb_v1_test.go, line 32 at r3 (raw file):
Previously, cameronnunez (Cameron Nunez) wrote…
func TestFormatCrdbV1Decode(t *testing.T) {For consistency, I suggest a slight change to the function name here.
Done.
pkg/util/log/format_crdb_v1_test.go, line 122 at r3 (raw file):
Previously, cameronnunez (Cameron Nunez) wrote…
Each instance of the string "clog_test.go" in
format_crdb_v1_test.goandtestdata/crdb_v1should be changed to the string "format_crdb_v1_test.go"
Done.
Release note: None
This simplifies the test and makes it more extensible. Release note: None
…entries Prior to this change, we did not have a public data structure able to represent the various fields available in a `crdb-v2` log entry: the existing `logpb.Entry` was not able to distinguish structured and non-structured entries, and did not have the ability to delimit the message and the stack trace. This patch extends `logpb.Entry` to make it more able to represent `crdb-v2` entries, at least for the purpose of extending `debug merge-log` towards conversion between logging formats. Additionally, the patch adds a *best effort* attempt at populating the new fields in the `crdb-v1` entry parser. This is best effort because the crdb-v1 parser is lossy so we cannot faithfully parse its entries reliably. Release note: None
Release note (cli change): When log entries are written to disk, the first few header lines written at the start of every new file now report the configured logging format.
Release note (bug fix): Previously, `--log='file-defaults: {format:
crdb-v1}'` was not handled properly. This has been fixed. This bug
existed since v21.1.0.
This increases troubleshootability. Release note: None
Understood! |
|
You still need to review and change your disposition. |
cameronnunez
approved these changes
Jun 7, 2021
👍 Done. |
|
awesome, thank you! bors r=cameronnunez |
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Jun 10, 2021
61378: log,debug: fix `logspy`, new HTTP API to change `vmodule` r=stevendanna,tbg,andreimatei a=miretskiy Fixes #61335. First 3 commits from #66096. **tldr: the logspy endpoint does not enable maximum verbosity any more, and now returns JSON entries. Use the new vmodule endpoint to configure verbosity if needed.** See the commit messages of the last 2 commits for details and rationales. Relevant user-facing changes: **Release note (security update):** All the logging output to files or network sinks was previously disabled temporarily while an operator was using the `/debug/logspy` HTTP API, resulting in lost entries and a breach of auditability guarantees. This behavior has been corrected. **Release note (bug fix):** Log entries are not lost any more while the `/debug/logspy` HTTP API is being used. This bug had existed since CockroachDB v1.1. **Release note (api change):** The `/debug/logspy` HTTP API has changed. The endpoint now returns JSON data by default. This change is motivated as follows: - the previous format, `crdb-v1`, cannot be parsed reliably. - using JSON entries guarantees that the text of each entry all fits on a single line of output (newline characters inside the messages are escaped). This makes filtering easier and more reliable. - using JSON enables the user to apply `jq` on the output, for example via `curl -s .../debug/logspy | jq ...` If the previous format is desired, the user can pass the query argument `&flatten=1` to the `logspy` URL to obtain the previous flat text format (`crdb-v1`) instead. **Release note (api change)**: The `/debug/logspy` API does not any more enable maximum logging verbosity automatically. To change the verbosity, use the new `/debug/vmodule` endpoint or pass the `&vmodule=` query parameter to the `/debug/logspy` endpoint. For example, suppose you wish to run a 20s logspy session: - Before: ``` curl 'https://.../debug/logspy?duration=20s&...' ``` - Now: ``` curl 'https://.../debug/logspy?duration=20s&vmodule=...' ``` OR ``` curl 'https://.../debug/vmodule?duration=22s&vmodule=...' curl 'https://.../debug/logspy?duration=20s' ``` As for the regular `vmodule` command-line flag, the maximum verbosity across all the source code can be selected with the pattern `*=4`. Note: at most one in-flight HTTP API request is allowed to modify the `vmodule` parameter. This maintain the invariant that the configuration restored at the end of each request is the same as when the request started. **Release note (api change)**: The new `/debug/vmodule` API makes it possible for an operator to configure the logging verbosity in a similar way as the SQL built-in function `crdb_internal.set_vmodule()`, or to query the current configuration as in `crdb_internal.get_vmodule()`. Additionally, any configuration change performed via this API can be automatically reverted after a configurable delay. The API forms are: - `/debug/vmodule` - retrieve the current configuration. - `/debug/vmodule?set=[vmodule config]&duration=[duration]` - change the configuration to `[vmodule config]` . The previous configuration at the time the `/debug/vmodule` request started is restored after `[duration]`. This duration, if not specified, defaults to twice the default duration of a `logspy` request (currently, the `logspy` default duration is 5s, so the `vmodule` default duration is 10s). If the duration is zero or negative, the previous configuration is never restored. 65648: sqlliveness/slstorage: break InternalExecutor depedency r=knz a=ajwerner There was no fundamental reason to rely on SQL for this low-level subsystem. Release note: None 66273: storageccl: don't update file registry when creating unencrypted files r=jbowens a=andyyang890 This patch changes PebbleFileRegistry to not store an entry for an unencrypted file. This should lead to a performance improvement since the file registry is rewritten every time it is updated. Fixes #65430. Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Andy Yang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Needed for #65633 and #61378.
cc @cockroachdb/server