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

[reconfigurator-cli] Customize log outputs #7322

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

jgallagher
Copy link
Contributor

My main goal with this PR was to fix the failing test on #7307 by emitting slog logs without timestamps (to avoid having to redact them in expectorate tests, and because they're not actually useful in a CLI). As a nice side effect, the output in a terminal is much nicer.

On main, I see this when planning a blueprint:

reconfig-main

I assume this is some bad interaction between slog-async / logs going to stderr / reedline.

On this branch, I now get:

reconfig-with-changes

* Dump them to stdout instead of stderr
* Omit timestamps (which should fix the test failure on #7307, where our
  expectorate tests don't know how to redact slog timestamps)
Copy link
Contributor

@andrewjstone andrewjstone left a comment

Choose a reason for hiding this comment

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

This looks so much better! Thanks @jgallagher

Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! I'm sorry to have left this particular piece of debt.

@jgallagher jgallagher merged commit 6efb34e into main Jan 8, 2025
17 checks passed
@jgallagher jgallagher deleted the john/reconfigurator-cli-slog branch January 8, 2025 20:15
@sunshowers
Copy link
Contributor

Thanks for doing this :)

It would be great to publish a good logger for CLI tools at some point. I'm partial to the general format Rust and Cargo use:

trace: ...
debug: ...
info: xyz
warn: something strange
error: whoops

where trace/debug/info are in bold, warn is in bold yellow, and error is in bold red. This is great though!

@davepacheco
Copy link
Collaborator

This is getting a bit far afield for this issue but: I've long been meaning to pull the logging config stuff that most of Omicron uses (e.g., https://docs.rs/dropshot/latest/dropshot/enum.ConfigLogging.html) out of dropshot. ConfigLogging could grow a new variant for CLI tools (or we could maybe change what StderrTerminal does) and then a lot of our stuff might pick up the new behavior with minimal changes. I'm not attached to this. I just want there to be a low-boilerplate way to do the thing that we want our stuff to do (and that's what ConfigLogging was intended to be).

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.

4 participants