Add basic features to CursiveLogger by blaisdellma · Pull Request #719 · gyscos/cursive

blaisdellma · 2023-03-06T07:32:16Z

Hello,

This is my attempt at following the first of my suggestions in #714 .

I tried to leave as much alone as I could so that there would be no breaking changes with as many useful features as possible. Anyone using the logger now shouldn't have to change anything and get the exact same behavior. There shouldn't be any breaking changes to the existing API, either in function signatures or behavior.

Logging would use two different filter levels, one each for logs generated internally by cursive and externally by whatever else. This solves the issue in #714 as the debug messages won't clog the log with all the redraws from scrolling (assuming the filter levels are set appropriately). These levels are set either directly or by the environment variables RUST_LOG and CURSIVE_LOG. I also added the ability to change the log queue size too. I hope this meets option 2 in #467 as a "good enough" feature set.

Two small examples:

cursive::logger::CursiveLogger::new()
    .with_int_log_level(log::LevelFilter::Warn)
    .with_ext_log_level(log::LevelFilter::Debug)
    .with_log_size(10000)
    .init();

cursive::logger::CursiveLogger::new()
    .with_env()
    .with_log_size(10000)
    .init();

The whole Box::leak(Box::new(self)) thing is to avoid having to add the std feature flag to log and change Cargo.toml. This is exactly how log handles set_boxed_logger internally.

gyscos · 2023-03-07T21:14:49Z

cursive-core/src/logger.rs

+    pub fn init(self) {
+        reserve_logs(self.log_size);
+        log::set_logger(Box::leak(Box::new(self))).unwrap();
+        log::set_max_level(log::LevelFilter::Trace);


We could set the maximum of (int, ext) levels to avoid logging trace if it wouldn't be seen anyway.

gyscos · 2023-03-07T21:19:06Z

cursive-core/src/logger.rs

+    /// Installs the logger with log. Calling twice will panic.
+    pub fn init(self) {
+        reserve_logs(self.log_size);
+        log::set_logger(Box::leak(Box::new(self))).unwrap();


We may want to use set_boxed_logger, which hides the leaking away so it feels less dirty (even though it's really the same):
https://docs.rs/log/latest/log/fn.set_boxed_logger.html

I guess a "leak-free" version would keep CursiveLogger zero-sized and rely on a global/singleton state instead, but that may be more complex, with quite limited benefits (prevents leaks if init() is called repeatedly).

Yeah, I originally had used set_boxed_logger but that's hidden behind the std feature flag for log and I didn't want to change the dependencies unless I had to.

As written, calling init() multiple times panics anyways, so I don't think we have to worry about leaks. Having everything kept in global state might not be a bad idea since we shouldn't expect anyone to create multiple CursiveLogger objects anyways.

I made another branch for this: cursivelogger_global. It is a bit more complicated and I don't know if I like the API ergonomics. One plus though is being able to change the log filter levels after initialization. I don't know if that's worth it.

Why the Mutex<Cell<...>> in that branch? Could a simple mutex work too? Or even RwLock, might make the read-only case faster.

I think I like this global branch! No feature change for the log dependency, uses lazy_static that we were already using...

Yeah, both work, you're right. I don't know what I was thinking with Mutex<Cell<...>>. Since LOGS is a Mutex anyway I'm not sure there's much of a speed difference.

I created a PR for cursivelogger_global. There is a tiny issue with set_log_size though.

gyscos · 2023-03-07T21:24:40Z

cursive-core/src/logger.rs

-    fn enabled(&self, _metadata: &log::Metadata) -> bool {
-        true
+    fn enabled(&self, metadata: &log::Metadata) -> bool {
+        if metadata.target().contains("cursive_core") {


Maybe we should be more restrictive, requiring this to be a prefix of the target?

gyscos · 2023-03-07T21:24:59Z

Thanks for the work! Just a couple nitpicks.

* Make CursiveLogger use globals * Avoid duplicate log queue reservations * Check log queue size on init, get rid of `LOG_SIZE`

blaisdellma · 2023-03-20T21:56:02Z

@gyscos
What do you think about the current state with the global logger?
What about the change to not having a set_log_size() function and just using reserve_logs()? (for reference)
Are there other changes/issues in the way of merging this?

gyscos

Maybe we could use VecDeque::with_capacity from lazy_static! rather than manually calling reserve_logs. It'd still only be called on the first use (so no wasted memory if users don't use this logger).

Not sure we even need to expose reserve_logs? 🤷

What about the change to not having a set_log_size() function and just using reserve_logs()? (blaisdellma#1 (comment))

Yeah let's keep the API minimal, any power user should use a real serious solution.

gyscos · 2023-03-27T18:08:39Z

cursive-core/src/logger.rs

+}
+
+/// Sets the internal log filter level.
+pub fn set_int_filter_level(level: log::LevelFilter) {


Nit: could we call it set_internal_filter_level, to prevent any ambiguity around int?

Let's also extend set_external_filter_level for consistency.

I don't care as much about the lazy static global as it's not public.

gyscos · 2023-03-27T18:12:21Z

cursive-core/src/logger.rs

+/// If `RUST_LOG` is set, then both internal and external log levels are set to match.
+/// If `CURSIVE_LOG` is set, then the internal log level is set to match with precedence over
+/// `RUST_LOG`.
+pub fn set_filter_levels_with_env() {


Nit

Suggested change

pub fn set_filter_levels_with_env() {

pub fn set_filter_levels_from_env() {

gyscos · 2023-03-27T18:13:55Z

cursive-core/src/logger.rs

+/// `RUST_LOG`.
+pub fn set_filter_levels_with_env() {
+    if let Ok(rust_log) = std::env::var("RUST_LOG") {
+        if let Ok(filter_level) = log::LevelFilter::from_str(&rust_log) {


I'd log (ha!) a warning in case we can't parse the level, rather than silently ignoring it.

This is meant to be called before the logger is initialized, so any logged warning would be ignored. I guess you could call it after initialization, but then when initializing the log::set_max_level() would be based on the default filter levels (which is fine, cause it defaults to trace). I'll add it anyway.

cursive-core/src/logger.rs

blaisdellma · 2023-03-28T14:39:34Z

Not sure we even need to expose reserve_logs? 🤷

I'm personally in favor of not making breaking changes to the API unless necessary. I don't know that it hurts to have it there, but it's your call.

gyscos · 2023-03-28T15:24:24Z

Thanks again for all the work and updates, and sorry for the trouble!

blaisdellma added 7 commits March 6, 2023 00:18

Add internal/external log levels

27bb3e5

Add variable log queue size

166e126

Remove changes to Cargo.toml

3af4e6d

Make env var based logging opt-in

22b6b8d

Add examples

ff0993d

Use LevelFilter from_str for parsing env vars

f60e591

rustfmt

3bfc086

gyscos reviewed Mar 7, 2023

View reviewed changes

blaisdellma and others added 4 commits March 7, 2023 17:11

set_max_level based on max int/ext log level

ca1950c

changed int/ext to prefix based

27701bd

Use set_boxed_logger (requires log std feature)

beb860d

Make CursiveLogger use global state (#1)

c8f9744

* Make CursiveLogger use globals * Avoid duplicate log queue reservations * Check log queue size on init, get rid of `LOG_SIZE`

gyscos reviewed Mar 27, 2023

View reviewed changes

cursive-core/src/logger.rs Show resolved Hide resolved

blaisdellma added 5 commits March 28, 2023 10:16

Changed to set_filter_levels_from_env()

2c54e4f

Expanded fn names from int/ext to internal/external

6bf3d4a

Combine lazy_static!

934ac2f

Use VecDeque::with_capacity()

e5f365f

Warn on failure to parse env vars

b122ba8

gyscos merged commit 2ca8223 into gyscos:main Mar 28, 2023

blaisdellma deleted the cursivelogger branch March 28, 2023 17:01

	pub fn set_filter_levels_with_env() {
	pub fn set_filter_levels_from_env() {

Conversation

blaisdellma commented Mar 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyscos Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyscos commented Mar 7, 2023

Uh oh!

blaisdellma commented Mar 20, 2023

Uh oh!

gyscos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blaisdellma commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gyscos commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gyscos Mar 8, 2023 •

edited

Loading

gyscos left a comment •

edited

Loading

blaisdellma commented Mar 28, 2023 •

edited

Loading