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

runtime toggleable debug logging #4750

Merged
merged 6 commits into from
Jan 8, 2024

Conversation

elliefm
Copy link
Contributor

@elliefm elliefm commented Nov 29, 2023

This adds an API such that config_debug (aka "debug:" in imapd.conf) can be toggled on and off at runtime, and hooks up a signal handler such that SIGUSR1 will toggle it. The test case tests this with imapd, but it should work the same for any Cyrus process (specifically, any process that calls both cyrus_init() and signals_add_handlers()).

You can already crudely toggle debug logging by changing debug: in imapd.conf and sending master a SIGHUP, but that a) affects all Cyrus process, b) only after they restart and reread the config, and c) also picks up other unrelated config changes. This PR allows toggling debug logging on or off for a specific process, while that process is already running, and without reloading the config generally.

SIGUSR1 is already used by cunit (for test timeout handling) and Cassandane (as part of hooking a debugger to a running test process), but isn't used by Cyrus itself. This usage shouldn't clash with the test systems' usages.

@elliefm
Copy link
Contributor Author

elliefm commented Nov 29, 2023

Pre-emptive "do not merge" cause by the time this is reviewed and I see it again we'll be in feature freeze. Will tag it for FM once approved.

@elliefm elliefm requested a review from wolfsage November 29, 2023 01:30
ksmurchison
ksmurchison previously approved these changes Nov 29, 2023
Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

LGTM.

If you don't want to have a special XNOOP command for testing, SELECT/EXAMINE already has a simple DEBUG log line when it completes

@elliefm
Copy link
Contributor Author

elliefm commented Nov 30, 2023

Oh great, thanks! I did have a brief look for something that was already debug logging, but didn't see that one. I'll see about switching the tests over to use it, and then get rid of XNOOP.

@elliefm elliefm force-pushed the v39/toggleable-debug branch from 7fb2bc2 to 542b5e5 Compare November 30, 2023 23:51
@elliefm
Copy link
Contributor Author

elliefm commented Nov 30, 2023

I've discarded the xnoop implementation, rewritten the test to use select instead, and added a comment to imapd.c explaining that cmd_select's debug log line is now load bearing, so that anyone trying to change it in the future doesn't get a surprise

@wolfsage
Copy link
Contributor

wolfsage commented Dec 4, 2023

This is amazing, thanks! I haven't played with it yet but would love to see it in action

@elliefm
Copy link
Contributor Author

elliefm commented Dec 5, 2023

Won't merge this till after the feature freeze, but I've tagged it include-in-fastmail for now so we can get it into our builds for @wolfsage to play with. Any issues, let me know, or just untag it.

@elliefm elliefm merged commit 037c478 into cyrusimap:master Jan 8, 2024
1 check passed
@elliefm elliefm deleted the v39/toggleable-debug branch January 8, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants