pkg/util/log: selectively apply file-defaults.format-options to stderr #113532
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
Code used to validate the stderr config parsing previously checked to see if the `crdb-v1-tty` format was being used when `auditable: true` was set. If it was, it changed the format to `crdb-v1-tty-count`. However, the `crdb-v1` log formats have long been deprecated, and the official logging documentation states: https://www.cockroachlabs.com/docs/stable/configure-logs#output-to-stderr > Note: The `format` parameter for `stderr` is set to `crdb-v2-tty` and cannot be changed. Given this, it not longer makes sense to have code that accounts for a stderr log format that's no longer valid. Instead, we should *always* assert the format to be `crdb-v2-tty` to align with what our documentation says. A following patch will add this assertion. Release note: none
abarganier
requested review from
a team and
dhartunian
and removed request for
a team
|
This change is |
abarganier
added
the
backport-23.2.x
Fixes: cockroachdb#113321 The stderr sink in the log config is only allowed to use the `crdb-v2-tty` format, and therefore, only `format-options` supported by the `crdb-v2-tty` format should be applied to the stderr sink. Unfortunately, a bug in the log config parse & validation code didn't follow this rule. The configuration that's part of the `file-defaults` is propagated to the stderr sink, and this included the `format-options`, even if they were only relevant to another log format type (e.g. `json`). This caused an error when trying to apply the options to the stderr log sink on startup, e.g.: ``` ERROR: unknown format option: "datetime-format" ``` To solve this problem, we should only propagate the `format-options` used in `file-defaults` to the stderr sink's config IFF the `file-defaults` format is of a `crdb-v2` variety. Since the stderr sink also uses the `crdb-v2-tty` format, we can only be sure that the `format-options` used in `file-defaults` is supported by the stderr sink if the `format` used in `file-defaults` is also part of `crdb-v2`. However, if `format-options` is explicitly defined within the `sinks.stderr` config, we need to be careful not to overwrite them with those defined in `file-defaults`. This patch accomplishes fixes for all these issues, and adds new tests to cover all these scenarios. Release note: none
dhartunian
approved these changes
Nov 2, 2023
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained
|
TFTR! bors r=dhartunian |
|
Build succeeded: |
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this pull request
Sep 27, 2024
Previously in cockroachdb#113532 we fixed the stderr sink log format to `crdb-v2-tty`. This was deemed to be a regression, as the stderr sink could previously be set to any available format. This commit reverts the restriction. The following are still true after this commit: - `format` in `file-defaults` is not propagated to `format` of the stderr sink. To set `stderr` format, one must explicitly define it in the `stderr` sink section of the log config. - `format-options` in `file-defaults` are only applied to stderr if the format of the stderr sink and that in file-defaults are compatible. Epic: none Fixes: cockroachdb#128665 Release note (ops change): Users may set the log format for thestderr sink by setting the `format` field in the logging config under the `stderr` sink section.
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this pull request
Sep 27, 2024
Previously in cockroachdb#113532 we fixed the stderr sink log format to `crdb-v2-tty`. This was deemed to be a regression, as the stderr sink could previously be set to any available format. This commit reverts the restriction. The following are still true after this commit: - `format` in `file-defaults` is not propagated to `format` of the stderr sink. To set `stderr` format, one must explicitly define it in the `stderr` sink section of the log config. - `format-options` in `file-defaults` are only applied to stderr if the format of the stderr sink and that in file-defaults are compatible. Epic: none Fixes: cockroachdb#128665 Release note (ops change): Users may set the log format for thestderr sink by setting the `format` field in the logging config under the `stderr` sink section.
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this pull request
Sep 27, 2024
Previously in cockroachdb#113532 we fixed the stderr sink log format to `crdb-v2-tty`. This was deemed to be a regression, as the stderr sink could previously be set to any available format. This commit reverts the restriction. The following are still true after this commit: - `format` in `file-defaults` is not propagated to `format` of the stderr sink. To set `stderr` format, one must explicitly define it in the `stderr` sink section of the log config. - `format-options` in `file-defaults` are only applied to stderr if the format of the stderr sink and that in file-defaults are compatible. Epic: none Fixes: cockroachdb#128665 Release note (ops change): Users may set the log format for the stderr sink by setting the `format` field in the logging config under the `stderr` sink section.
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this pull request
Sep 27, 2024
Previously in cockroachdb#113532 we fixed the stderr sink log format to `crdb-v2-tty`. This was deemed to be a regression, as the stderr sink could previously be set to any available format. This commit reverts the restriction. The following are still true after this commit: - `format` in `file-defaults` is not propagated to `format` of the stderr sink. To set `stderr` format, one must explicitly define it in the `stderr` sink section of the log config. - `format-options` in `file-defaults` are only applied to stderr if the format of the stderr sink and that in file-defaults are compatible. Epic: none Fixes: cockroachdb#128665 Release note (ops change): Users may set the log format for the stderr sink by setting the `format` field in the logging config under the `stderr` sink section.
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this pull request
Sep 27, 2024
Previously in cockroachdb#113532 we fixed the stderr sink log format to `crdb-v2-tty`. This was deemed to be a regression, as the stderr sink could previously be set to any available format. This commit reverts the restriction. The following are still true after this commit: - `format` in `file-defaults` is not propagated to `format` of the stderr sink. To set `stderr` format, one must explicitly define it in the `stderr` sink section of the log config. - `format-options` in `file-defaults` are only applied to stderr if the format of the stderr sink and that in file-defaults are compatible. Epic: none Fixes: cockroachdb#128665 Release note (ops change): Users may set the log format for the stderr sink by setting the `format` field in the logging config under the `stderr` sink section.
This was referenced Sep 27, 2024
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this pull request
Sep 27, 2024
Previously in cockroachdb#113532 we fixed the stderr sink log format to `crdb-v2-tty`. This was deemed to be a regression, as the stderr sink could previously be set to any available format. This commit reverts the restriction. The following are still true after this commit: - `format` in `file-defaults` is not propagated to `format` of the stderr sink. To set `stderr` format, one must explicitly define it in the `stderr` sink section of the log config. - `format-options` in `file-defaults` are only applied to stderr if the format of the stderr sink and that in file-defaults are compatible. Epic: none Fixes: cockroachdb#128665 Release note (ops change): Users may set the log format for the stderr sink by setting the `format` field in the logging config under the `stderr` sink section.
craig bot
pushed a commit
that referenced
this pull request
Sep 27, 2024
131529: util/log/logconfig: allow `format` for stderr log sink r=xinhaoz a=xinhaoz Previously in #113532 we fixed the stderr sink log format to `crdb-v2-tty`. This was deemed to be a regression, as the stderr sink could previously be set to any available format. This commit reverts the restriction. The following are still true after this commit: - `format` in `file-defaults` is not propagated to `format` of the stderr sink. To set `stderr` format, one must explicitly define it in the `stderr` sink section of the log config. - `format-options` in `file-defaults` are only applied to stderr if the format of the stderr sink and that in file-defaults are compatible. Epic: none Fixes: #128665 Release note (ops change): Users may set the log format for the stderr sink by setting the `format` field in the logging config under the `stderr` sink section. Co-authored-by: Xin Hao Zhang <[email protected]>
xinhaoz
added a commit
that referenced
this pull request
Sep 27, 2024
Previously in #113532 we fixed the stderr sink log format to `crdb-v2-tty`. This was deemed to be a regression, as the stderr sink could previously be set to any available format. This commit reverts the restriction. The following are still true after this commit: - `format` in `file-defaults` is not propagated to `format` of the stderr sink. To set `stderr` format, one must explicitly define it in the `stderr` sink section of the log config. - `format-options` in `file-defaults` are only applied to stderr if the format of the stderr sink and that in file-defaults are compatible. Epic: none Fixes: #128665 Release note (ops change): Users may set the log format for the stderr sink by setting the `format` field in the logging config under the `stderr` sink section.
cthumuluru-crdb
pushed a commit
to cthumuluru-crdb/cockroach
that referenced
this pull request
Oct 1, 2024
Previously in cockroachdb#113532 we fixed the stderr sink log format to `crdb-v2-tty`. This was deemed to be a regression, as the stderr sink could previously be set to any available format. This commit reverts the restriction. The following are still true after this commit: - `format` in `file-defaults` is not propagated to `format` of the stderr sink. To set `stderr` format, one must explicitly define it in the `stderr` sink section of the log config. - `format-options` in `file-defaults` are only applied to stderr if the format of the stderr sink and that in file-defaults are compatible. Epic: none Fixes: cockroachdb#128665 Release note (ops change): Users may set the log format for the stderr sink by setting the `format` field in the logging config under the `stderr` sink section.
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.
Fixes: #113321
The stderr sink in the log config is only allowed to use the
crdb-v2-ttyformat, and therefore, onlyformat-optionssupported bythe
crdb-v2-ttyformat should be applied to the stderr sink.Unfortunately, a bug in the log config parse & validation code didn't
follow this rule. The configuration that's part of the
file-defaultsis propagated to the stderr sink, and this included the
format-options, even if they were only relevant to another log formattype (e.g.
json). This caused an error when trying to apply theoptions to the stderr log sink on startup, e.g.:
To solve this problem, we should only propagate the
format-optionsused in
file-defaultsto the stderr sink's config IFF thefile-defaultsformat is of acrdb-v2variety. Since the stderr sinkalso uses the
crdb-v2-ttyformat, we can only be sure that theformat-optionsused infile-defaultsis supported by the stderr sinkif the
formatused infile-defaultsis also part ofcrdb-v2.However, if
format-optionsis explicitly defined within thesinks.stderrconfig, we need to be careful not to overwrite them withthose defined in
file-defaults.This patch accomplishes fixes for all these issues, and adds new tests
to cover all these scenarios.
Release note: none