Repository navigation
Remove Memento and add support for Logging.jl - #99
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #99 +/- ##
==========================================
+ Coverage 90.31% 90.40% +0.08%
==========================================
Files 10 11 +1
Lines 940 959 +19
==========================================
+ Hits 849 867 +18
- Misses 91 92 +1
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| @@ -0,0 +1,56 @@ | |||
| const _LOGGER = Ref{Logging.ConsoleLogger}() | |||
There was a problem hiding this comment.
This is the main file for the logging.
The _LOGGER is a const but it is a Ref because Logging.ConsoleLogger is immutable; we want to be able to change the logging level at runtime.
| silence() = logger_config!("error") | ||
|
|
||
| function _meta_formatter(level::Logging.LogLevel, _module, args...) | ||
| return Logging.default_logcolor(level), "$(_module) | $level]:", "" |
There was a problem hiding this comment.
Here is where we can change how the log messages are formatted. For now, it just prefaces the log message with something like [InfrastructureModels | Warn]: msg. (Note that the opening [ is added automatically by Logging.jl.)
In future we could add more information like file and line number. Or we could standardise this function into InfrastructureModels.meta_formatter so that other InfrastructureModels packages could all use the same formatting. For now, we're just copying this function into each package.
|
|
||
| function logger_config!(level::Logging.LogLevel) | ||
| _LOGGER[] = | ||
| Logging.ConsoleLogger(stdout, level; meta_formatter = _meta_formatter) |
There was a problem hiding this comment.
This function updates the global _LOGGER.
| `"warn"`, `"info"`, or `"debug"`. | ||
| """ | ||
| function logger_config!(level::String) | ||
| return getfield(Logging, level |> titlecase |> Symbol) |> logger_config! |
There was a problem hiding this comment.
This is a bit messy. The other option is a big if level == "error" ... end.
If you miss-type, you get this error:
julia> getfield(Logging, "err" |> titlecase |> Symbol)
ERROR: UndefVarError: `Err` not defined in `Logging`
Suggestion: check for spelling errors or missing imports.
|
|
||
| function _log_if_level(f, level, logger = _LOGGER[]) | ||
| if level >= Logging.min_enabled_level(logger) | ||
| Logging.with_logger(f, logger) |
There was a problem hiding this comment.
The if level >= ... check is a cheap check to see if the logger should run. If we shouldn't log this message, then we can skip with_logger entirely (it has some overhead and allocations).
| macro _error(msg) | ||
| return quote | ||
| $_log_if_level(() -> @error($msg), $(Logging.Error)) | ||
| error($msg) |
There was a problem hiding this comment.
Note that @error doesn't throw. But Memento.error did. To preserve backwards compatibility we add an explicit error(msg) here.
| function logger_config!(level; kwargs...) | ||
| Memento.config!(_LOGGER, level, kwargs...) | ||
| function __init__() | ||
| logger_config!("info") |
There was a problem hiding this comment.
Same as Memento.register, we need to enable the logger on __init__.
- Use the same approach as in InfrastructureModels: <lanl-ansi/InfrastructureModels.jl#99>. - Lower the compatibility requirement for Logging to 1.10 to support Julia LTS.
Closes #98
This PR removes Memento in favour of the Logging.jl standard library. The API for
silence!andlogger_config!are unchanged.There is a new file
src/core/logging.jlwhich contains the logic for logging.Throughout the rest of the package, the changes are:
Memento.error(_LOGGER,becomes@_error(Memento.warn(_LOGGER,becomes@_warn(Memento.info(_LOGGER,becomes@_info(Memento.debug(_LOGGER,becomes@_debug(The benefit of this approach is that it is self-contained and backward compatible within a package. It doesn't require the entire InfrastructureModels ecosystem to move as one from Memento to Logging.jl.
We looked into other ways of writing the logger, #98, lanl-ansi/PowerModels.jl#997 before ultimately deciding on this approach.
We also investigated updating Memento, but it commits type-piracy on this macro: https://github.com/invenia/Memento.jl/blob/58a92c6d688a5372e04c3d05c6da7a0cf3430a94/src/memento_test.jl#L119
We'd likely need to remove it, or rename it to something like
@memento_test_throws. This would be a breaking change that we'd need to update every InfrastructureModels package for any way, so we might as well bite the bullet and remove it as a dependency. This should save us from any future issues in the long term.PowerModelsDistribution
Our approach is different to PMD:
https://github.com/lanl-ansi/PowerModelsDistribution.jl/blob/main/src/core/logging.jl
They muck with the global logger, which would affect all packages using logging. As a single package it's okay. And it doesn't conflict with the approach taken in this PR. But the logic is not correct if multiple packages adopted the logging style of PowerModelsDistribution. Here's an example of where things can go wrong: lanl-ansi/PowerModels.jl#997 (comment) (Changing the logger mutates global state; multiple packages modifying the same global state can get into issues.)
Other PRs