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

util/log/logconfig: allow format for stderr log sink #131529

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented 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.

@xinhaoz xinhaoz requested review from a team as code owners September 27, 2024 17:39
@xinhaoz xinhaoz requested review from kyle-a-wong, arjunmahishi and aa-joshi and removed request for a team September 27, 2024 17:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz requested a review from dhartunian September 27, 2024 17:39
@xinhaoz xinhaoz added backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Sep 27, 2024
@xinhaoz xinhaoz force-pushed the allow-stderr-formats branch 3 times, most recently from 4eb0fb3 to afa0d8e Compare September 27, 2024 17:42
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @arjunmahishi, @kyle-a-wong, and @xinhaoz)


-- commits line 2 at r1:
nit: >


-- commits line 22 at r1:
nit space.


pkg/util/log/logconfig/testdata/validate line 120 at r1 (raw file):

    filter: NONE
    format-options:
      foo: bar

just for my understanding: why were these propagating prior to this change?


pkg/util/log/logconfig/testdata/validate line 295 at r1 (raw file):

  stderr:
    filter: NONE
    format: json

I don't see the format options here


pkg/util/log/logconfig/testdata/validate line 322 at r1 (raw file):

    format-options:
      datetime-format: rfc3339
      datetime-timezone: America/New_York

don't we expect the format options to be omitted?


pkg/util/log/logconfig/testdata/validate line 366 at r1 (raw file):

  stderr:
    filter: NONE
    format: crdb-v1-tty

can you add a similar test case that uses a non -tty format. I could imagine a restriction where stderr only accepts tty formats.

@xinhaoz xinhaoz force-pushed the allow-stderr-formats branch from afa0d8e to a9b9b85 Compare September 27, 2024 18:16
Copy link
Member Author

@xinhaoz xinhaoz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @arjunmahishi, @dhartunian, and @kyle-a-wong)


-- commits line 2 at r1:

Previously, dhartunian (David Hartunian) wrote…

nit: >

Removed.


-- commits line 22 at r1:

Previously, dhartunian (David Hartunian) wrote…

nit space.

Fixed.


pkg/util/log/logconfig/testdata/validate line 295 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

I don't see the format options here

Fixed, see other comment.


pkg/util/log/logconfig/testdata/validate line 322 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

don't we expect the format options to be omitted?

Oops I totally flipped the boolean I was supposed to be checking 🤦‍♀️ 🤦‍♀️ Sorry about that, everything should be fixed now.

@xinhaoz xinhaoz requested a review from dhartunian September 27, 2024 18:22
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @aa-joshi, @arjunmahishi, @kyle-a-wong, and @xinhaoz)


pkg/util/log/logconfig/testdata/validate line 357 at r3 (raw file):

  max-group-size: 100MiB

# Check that stderr can accept format options.

nit: this test is not format option related.

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 xinhaoz force-pushed the allow-stderr-formats branch from a9b9b85 to ccd3609 Compare September 27, 2024 18:57
@xinhaoz xinhaoz changed the title util/log/logconfig>: allow format for stderr log sink util/log/logconfig: allow format for stderr log sink Sep 27, 2024
@xinhaoz
Copy link
Member Author

xinhaoz commented Sep 27, 2024

TFTR!
bors r+

@xinhaoz xinhaoz removed backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Sep 27, 2024
@craig craig bot merged commit 946ebda into cockroachdb:master Sep 27, 2024
23 checks passed
@xinhaoz xinhaoz deleted the allow-stderr-formats branch October 7, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the format of stderr file configurable
3 participants