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

cli: change the default log config for tests to only report storage warnings/errors on stderr #69765

Closed
knz opened this issue Sep 2, 2021 · 1 comment · Fixed by #70052
Closed
Labels
A-testing Testing tools and infrastructure C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-server-and-security DB Server & Security

Comments

@knz
Copy link
Contributor

knz commented Sep 2, 2021

The default log config used in unit tests only has two modes:

  • either redirect all the log events to just 1 file inside the directory configured by log.Scope, or
  • if -v -show-logs is specified, or the test does not use log.Scope, all log events go to stderr.

Folk have found the following shortcomings with this situation:

  1. in the simple case, when a test fails all the logging output is merged into a single log file. This makes investigations somewhat difficult because the combined output from all channels is extremely verbose.

    The preference would be to ensure that multiple files are used instead. At least the STORAGE channel should be redirected to a different file in this "silent" case.

    In that case, we want the mechanism described also in cli: change the default logging configuration for regular server #69781: send the messages to a dedicated file, but copy WARNING+ messages to the main log file shared with the DEV Channel.

    (Do this both for STORAGE and HEALTH)

    PR: util/log: split test logs by default into multiple files #69958

  2. in the -show-logs case, they would prefer that all the STORAGE messages do not get reported on stderr by default, because most tests do not require storage-level investigations and the STORAGE channel is rather noisy.

    However, the request is not to remove STORAGE entirely from stderr either. The preference is to ensure that only WARNING/ERROR/FATAL messages from STORAGE get reported on stderr, and the rest in files.

    Andrei also recommends to do the same for the HEALTH channel.

    PR: util/log: use some files when -show-logs is specified #70052

  3. in some cases, they would prefer to have an option to fine tune the configuration. Currently, the log config is static per build and cannot be changed per test invocation.

    We could thus perhaps add an optional flag to override the default log config for unit tests.

    PR: util/log: make it possible to customize the log config in unit tests #69954

  4. the stderr configuration when files are enabled include all events at severity ERROR or above. However, several tests terminate servers in unfriendly ways, which generates errors in logs, but these errors are not really interesting.

    We want to change the stderr config to only report messages at severity FATAL. If there are errors, the person can go and investigate the generated files.

    PR: util/log: don't report ERROR events on stderr by default in test config #69955

The most pressing problem is point (2) above; followed by (1), then (4), and then (3) as a distant third.

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-testing Testing tools and infrastructure labels Sep 2, 2021
@blathers-crl blathers-crl bot added the T-server-and-security DB Server & Security label Sep 2, 2021
@knz
Copy link
Contributor Author

knz commented Sep 2, 2021

Relevant in this issue, this comment by @andreimatei here #69701 (comment)

we may want to redirect the HEALTH channel and its structured events to a separate file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Testing tools and infrastructure C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-server-and-security DB Server & Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant