From 93f189a4b59dd421887539a84b55753ed358086c Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Fri, 27 Sep 2024 13:33:37 -0400 Subject: [PATCH] util/log/logconfig>: allow `format` for stderr log sink 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 thestderr sink by setting the `format` field in the logging config under the `stderr` sink section. --- pkg/util/log/logconfig/config.go | 3 -- pkg/util/log/logconfig/testdata/validate | 42 ++++++++++++++++++++---- pkg/util/log/logconfig/validate.go | 28 +++++++++++----- 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/pkg/util/log/logconfig/config.go b/pkg/util/log/logconfig/config.go index 5ce61a5a93ec..283e4d3230a7 100644 --- a/pkg/util/log/logconfig/config.go +++ b/pkg/util/log/logconfig/config.go @@ -30,9 +30,6 @@ import ( const DefaultFileFormat = `crdb-v2` // DefaultStderrFormat is the entry format for stderr sinks. -// NB: The format for stderr is always set to `crdb-v2-tty`, -// and cannot be changed. We enforce this in the validation step. -// See: https://www.cockroachlabs.com/docs/stable/configure-logs#output-to-stderr const DefaultStderrFormat = `crdb-v2-tty` // DefaultFluentFormat is the entry format for fluent sinks diff --git a/pkg/util/log/logconfig/testdata/validate b/pkg/util/log/logconfig/testdata/validate index 088df190120a..b5be129d0130 100644 --- a/pkg/util/log/logconfig/testdata/validate +++ b/pkg/util/log/logconfig/testdata/validate @@ -116,8 +116,6 @@ sinks: foo: bar stderr: filter: NONE - format-options: - foo: bar capture-stray-errors: enable: true dir: /default-dir @@ -254,7 +252,7 @@ capture-stray-errors: dir: /default-dir max-group-size: 100MiB -# Check that file-defaults format options are transferred to stderr if using a crdb-v2 format. +# Check that file-defaults format options are transferred to stderr if using the same format (crdb-v2). yaml file-defaults: format: crdb-v2 @@ -269,18 +267,19 @@ sinks: timezone: america/new_york stderr: filter: NONE - format-options: - timezone: america/new_york capture-stray-errors: enable: true dir: /default-dir max-group-size: 100MiB -# Check that file-defaults format options are NOT transferred to stderr if NOT using a crdb-v2 format. +# Check that file-defaults format options are transferred to stderr if stderr is using the same format (json). yaml file-defaults: format: json format-options: {datetime-format: rfc3339, datetime-timezone: America/New_York} +sinks: + stderr: + format: json ---- sinks: file-groups: @@ -293,6 +292,34 @@ sinks: datetime-timezone: America/New_York stderr: filter: NONE + format: json +capture-stray-errors: + enable: true + dir: /default-dir + max-group-size: 100MiB + +# Check that file-defaults format options are NOT transferred to stderr if stderr is NOT using the same format +# as file-defaults. +# Note that here, we are using the default stderr format crdb-v2-tty. +yaml +file-defaults: + format: json + format-options: {datetime-format: rfc3339, datetime-timezone: America/New_York} +---- +sinks: + file-groups: + default: + channels: {INFO: all} + filter: INFO + format: json + format-options: + datetime-format: rfc3339 + datetime-timezone: America/New_York + stderr: + filter: NONE + format-options: + datetime-format: rfc3339 + datetime-timezone: America/New_York capture-stray-errors: enable: true dir: /default-dir @@ -323,7 +350,7 @@ capture-stray-errors: dir: /default-dir max-group-size: 100MiB -# Check that stderr always uses crdb-v2-tty format, even if we try to set it to an invalid format +# Check that stderr can accept format options. yaml sinks: stderr: @@ -336,6 +363,7 @@ sinks: filter: INFO stderr: filter: NONE + format: crdb-v1-tty capture-stray-errors: enable: true dir: /default-dir diff --git a/pkg/util/log/logconfig/validate.go b/pkg/util/log/logconfig/validate.go index 86131f116bf1..9265f2e202ad 100644 --- a/pkg/util/log/logconfig/validate.go +++ b/pkg/util/log/logconfig/validate.go @@ -177,17 +177,12 @@ func (c *Config) Validate(defaultLogDir *string) (resErr error) { c.Sinks.Stderr.Criticality = &bt } c.Sinks.Stderr.Auditable = nil - // The format parameter for stderr is set to `crdb-v2-tty` and cannot be changed. - // See docs: https://www.cockroachlabs.com/docs/stable/configure-logs#output-to-stderr - if *c.Sinks.Stderr.Format != DefaultStderrFormat { - f := DefaultStderrFormat - c.Sinks.Stderr.Format = &f - } + // FormatOptions are format-specific. We should only copy them over to StdErr from - // FileDefaults if FileDefaults is also making use of a crdb-v2 format. Otherwise, + // FileDefaults if FileDefaults specifies the same format as the stderr sink. Otherwise, // we are likely to error when trying to apply an unsupported format option. if c.FileDefaults.CommonSinkConfig.Format != nil && - !strings.Contains(*c.FileDefaults.CommonSinkConfig.Format, "v2") && + canShareFormatOptions(*c.FileDefaults.CommonSinkConfig.Format, *c.Sinks.Stderr.Format) && !stdErrFormatOptionsOriginallySet { c.Sinks.Stderr.CommonSinkConfig.FormatOptions = map[string]string{} } @@ -538,3 +533,20 @@ func propagateDefaults(target, source interface{}) { } } } + +// canShareFormatOptions returns true if f1 and f2 can share format options. +// See log.FormatParsers for full list of supported formats. +// Examples: +// +// canShareFormatOptions("json", "json") => true +// canShareFormatOptions("crdb-v2", "crdb-v2-tty") => true +// canShareFormatOptions("crdb-v2", "json") => false +func canShareFormatOptions(f1, f2 string) bool { + if f1 == f2 { + return true + } + if strings.HasPrefix(f1, f2+"-") || strings.HasPrefix(f2, f1+"-") { + return true + } + return false +}