log: change comptime log to .debug and add runtime filtering #2539
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matklad
reviewed
Dec 5, 2024
src/clients/c/tb_client.zig
Outdated
| // Other messages are silently dropped if no logging callback is specified - unless they're | ||
| // warn or err. The value in having those for debugging is too high to silence them, even | ||
| // until client libraries catch up and implement a callback handler. | ||
| const callback = if (message_level == .debug and logging.debug) |
There was a problem hiding this comment.
meant to say
Suggested change
| const callback = if (message_level == .debug and logging.debug) | |
| const callback = if (message_level == .debug and !logging.debug) |
perhaps?
Could you also separate out this if to be a distinct statement? Right now, the if is quite busy and feels like it handles separate concerns?
Maybe avoid the ternary at all?
if (message_level == .debug and !logging.debug) {
return;
}
if (logging.callback == null and
(message_level == .warn or message_level === .err)) {
std.log.defaultLog(message_level, scope, format, args);
return;
}
const callback = logging.callback orelse return;There was a problem hiding this comment.
Whoops! Good catch.
Yeah agreed, I thought originally there might be a tiny performance benefit from a ternary, but I'm probably wrong and client logging is called way less than replica logging so it's less relevant too.
Previously, .debug log messages were filtered out at comptime. While nice from a binary size and performance (more on that later) point of view, this meant that a user couldn't specify `--log-debug` or similar and would need a new binary to get debug logging. Make it so that we do runtime log filtering instead. The performance penalty for this was negligible in testing; both microbenchmarks and the actual benchmark. The one downside is binary bloat: this increases the size of a release build by ~300KiB. To use, specify `--log-debug --experimental` on the CLI when starting a replica. It's currently only supported for `start`. Scaffolding for clients is there too, with filtering of the messages before they cross FFI and requiring an additional flag to be set. Actually setting this flag from any client libraries is still unimplemented - but this makes it no harder than before to change client logging (edit code & recompile).
matklad
approved these changes
Dec 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, .debug log messages were filtered out at comptime. While nice from a binary size and performance (more on that later) point of view, this meant that a user couldn't specify
--log-debugor similar and would need a new binary to get debug logging.Make it so that we do runtime log filtering instead. The performance penalty for this was negligible in testing; both microbenchmarks and the actual benchmark.
The one downside is binary bloat: this increases the size of a release build by ~300KiB.
To use, specify
--log-debug --experimentalon the CLI when starting a replica.--log-debuginstead of--log=...because I feel we don't want to allow levels quieter than.infoin any case.It's currently only supported for
start. Scaffolding for clients is there too, with filtering of the messages before they cross FFI and requiring an additional flag to be set. Actually setting this flag from any client libraries is still unimplemented - but this makes it no harder than before to change client logging (edit code & recompile).Things like the VOPR and fuzzers use their own way of setting logging, and are unaffected.