daroczig / logger

A lightweight, modern and flexible, log4j and futile.logger inspired logging utility for R
https://daroczig.github.io/logger
280 stars 41 forks source link

Make it easier to set the log threshold with an environment variable. #73

Closed sellorm closed 6 months ago

sellorm commented 3 years ago

I'm a big fan of setting log thresholds using environment variables so that people don't have to edit my apps to change the level.

This is really useful in production environments where the people running my code often do not know R and should not be editing any of the scripts, but who may need to change the log threshold of the application prior to execution.

In order to get this to work with logger, I end up having to do something like this:

log_threshold_from_env_var <- function(){
  log_level_env_var <- Sys.getenv("LOG_LEVEL", "INFO")
  log_levels <- c("FATAL", "ERROR", "WARN", "SUCCESS", "INFO", "DEBUG", "TRACE")
  if (! (log_level_env_var %in% log_levels)){
    err_msg <- "The LOG_LEVEL environment variable must be either unset, or set to a valid log level" 
    stop(err_msg)
  }
  get(log_level_env_var)
}

Which I can use as follows:

library(logger)
log_threshold(log_threshold_from_env_var())

This still defaults to INFO if the env var is unset.

Please could something like this be added to logger to make setting the log threshold from an environment variable more straightforward.

If there's a better way of doing this already, it's not clear from the documentation, so then I'd change this to a documentation request.

daroczig commented 3 years ago

Thanks a lot for the suggestion, I like the idea and easy to implement -- but can you please confirm which approach are you actually looking for?

Eg (1) is trivial and I can make that happen today, (3) is also doable, but need some time to figure out how to do that properly.

sellorm commented 3 years ago

Ha! Yes, sorry, I should have been clearer about that.

Any of those options would be great.

However, Option 1 is very explicit about what's happening and it also doesn't change any existing behaviours, so I'd perhaps prefer that in the short term.

The third option would be really nice, but it feels like a fairly significant change from the way the package currently works. This feels like more of a long-term goal.

Tamaraa01x commented 1 year ago

[]()```

- [ ] [``]()

AlexAxthelm commented 7 months ago

@daroczig This issue is something I'm also dealing with, and I'd be happy to help with a PR, but I'd like to know what you think would be a good approach.

My inclination would be to use .onLoad() to set the default level when the package is loaded, along the lines of:

.onLoad <- function(libname, pkgname) {
  log_threshold(Sys.getenv("LOG_LEVEL", "INFO"))
}

This would set the default threshold for any subsequent loggers (which could be set as desired using the normal log_threshold(<level>, index = <index>)), and wouldn't impose a performance hit. Writing tests could be tricky,(I've never tried detach()ing a package inside tests).

Overall, I'm wondering if you see any issues with this, since it would be altering the current default behavior.

I'd also define an option (logger.ignore_envvar?) that would allow the user to ignore $LOG_LEVEL (re-enable current behavior), but this could be logger.use_envvar if you want to not change any behaviors implicitly.

daroczig commented 6 months ago

Sorry that it took so long to get back to this .. but just planning a CRAN release, so I thought I should quickly check if the following works for you: https://github.com/daroczig/logger/pull/145/files

I'd highly appreciate any feedback before the end of this week, when I'm planning to submit a new version to CRAN. No hard feelings if you cannot get a chance to take a look after that many years, though :)

AlexAxthelm commented 6 months ago

That looks great to me!

daroczig commented 6 months ago

Implemented at #145