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.

Sign up for GitHub

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

Jump to bottom

pkg/util/log: timezone options parsed using crdb-v2, even when format is json #113321

Closed
abarganier opened this issue Oct 30, 2023 · 4 comments · Fixed by #113532
Closed

pkg/util/log: timezone options parsed using crdb-v2, even when format is json #113321

abarganier opened this issue Oct 30, 2023 · 4 comments · Fixed by #113532
Assignees
@abarganier
Labels
A-observability-inf branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.

Comments

@abarganier
Copy link
Contributor

abarganier commented Oct 30, 2023 •
edited by cockroach-jira-scripts
Loading

Describe the problem

#104265 introduced some new log config fields to add configurability around timezones and datetime formats used in logs.

the available options should be:

  • crdb-v1 & crdb-v2: timezone
  • json: datetime-timezone and datetime-format

However, the following config causes an error of: ERROR: unknown format option: "datetime-format"

file-defaults:
  format: json
  format-options: { datetime-format: rfc3339 }

After some debugging, I found that this is because the crdb-v2 format options parser is being used, even when we state the format: json.

Expected behavior
The format-options should be parsed properly for the json log format.

Jira issue: CRDB-32868
@abarganier abarganier added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability-inf labels Oct 30, 2023
@abarganier abarganier self-assigned this Oct 30, 2023
@abarganier abarganier added the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Oct 30, 2023
@blathers-crl
Copy link

blathers-crl bot commented Oct 30, 2023

Hi @abarganier, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl
Copy link

blathers-crl bot commented Oct 31, 2023

Hi @dhartunian, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@dhartunian dhartunian added the branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 label Oct 31, 2023
@exalate-issue-sync exalate-issue-sync bot mentioned this issue Oct 31, 2023
Closed
@abarganier
Copy link
Contributor Author

abarganier commented Oct 31, 2023 •
edited
Loading

It appears that this bug happens with any format-option used when specifying a json format in the file-defaults.

For example, this also errored:

file-defaults:
  format: json
  format-options: {tag-style: verbose}

I believe the error is actually occurring when attempting to apply the default stderr config - we're not even hitting the file sink config apply step.

@abarganier
Copy link
Contributor Author

abarganier commented Oct 31, 2023 •
edited
Loading

We propagate the CommonSinkConfig component of the file-defaults into the stderr config:

cockroach/pkg/util/log/logconfig/validate.go

Line 173 in 250bb36

propagateCommonDefaults(&c.Sinks.Stderr.CommonSinkConfig, c.FileDefaults.CommonSinkConfig)

Why doesn't the format get propagated along with it, while the format-options do?

EDIT: Ah! I see now: 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.

If that's the case, then we should only propagate the format options if they're supported by the crdb-v2-tty format.

With some added logging, here are the results of how the format and format-options get propagated:

Propagating...
    File Format: json, Stderr Format: crdb-v2-tty
    File Format-options: map[datetime-format:rfc3339 datetime-timezone:America/New_York], Stderr Format-options: map[]
Finished propagating...
    File Format: json, Stderr Format: crdb-v2-tty
    File Format-options: map[datetime-format:rfc3339 datetime-timezone:America/New_York], Stderr Format-options: map[datetime-format:rfc3339 datetime-timezone:America/New_York]

@abarganier abarganier mentioned this issue Oct 31, 2023
Merged
craig bot pushed a commit that referenced this issue Nov 2, 2023
@abarganier @msbutler
Merge #113532 #113534
4dad7f2 
113532: pkg/util/log: selectively apply file-defaults.format-options to stderr r=dhartunian a=abarganier

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


113534: roachtest: test AOST restore in backup-restore/* roachtests r=renatolabs a=msbutler

This patch allows the backup-restore driver to run and validate AOST restores from revision history backups. If the driver created a revision history backup, there's a 50% chance it will restore from an AOST between the full backup end time and the last incremental start time. A future patch will allow for AOST restores from non-revision history backups.

Epic: none

Release note: none

Co-authored-by: Alex Barganier <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
@craig craig bot closed this as completed in c0addf4 Nov 2, 2023
blathers-crl bot pushed a commit that referenced this issue Nov 2, 2023
@abarganier
pkg/util/log: selectively apply file-defaults.format-options to stderr
380a2f6 
Fixes: #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
@blathers-crl blathers-crl bot mentioned this issue Nov 2, 2023
Merged
abarganier added a commit that referenced this issue Nov 6, 2023
@abarganier
pkg/util/log: selectively apply file-defaults.format-options to stderr
b03fe9f 
Fixes: #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 (bug fix): A bug in the log config code prevented users
from setting the `datetime-format` and `datetime-timezone` log format
options (set via the `format-options` structure) within their log
config. Specifically, when users tried to use these options in
`file-defaults` with any `json` type log format, the log config was
previously unable to be parsed due to validation errors.

This was because the `file-defaults.format-options` were propagated to
the `sinks.stderr.format-options`. `sinks.stderr` only supports a format
of `crdb-v2-tty`. Therefore, the incorrectly propagated
`format-options`, which are only supported by the `json` log format,
were identified as not being supported when validating `sinks.stderr`.

With this patch, the `file-defaults.format-options` are only propagated
to `sinks.stderr.format-options` if both of these conditions are true:
1. `file-defaults.format` is one of `crdb-v2` or `crdb-v2-tty`.
2. `sinks.stderr.format-options` are not explicitly set in the log
   config.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@abarganier abarganier

Labels
A-observability-inf branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

pkg/util/log: selectively apply file-defaults.format-options to stderr abarganier/cockroach
2 participants
@dhartunian @abarganier