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

Import glue #198

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Import glue #198

wants to merge 2 commits into from

Conversation

hadley
Copy link
Collaborator

@hadley hadley commented Aug 18, 2024

I'm not sure I got all the docs, but hopefully I got most of them.

hadley added 2 commits August 18, 2024 16:40
I'm not sure I got all the docs, but hopefully I got most of them.
@tdeenes
Copy link
Contributor

tdeenes commented Aug 23, 2024

@hadley @daroczig Gergo and Hadley, with all respect (esp. regarding the impressive series of your recent PRs, Hadley), could we keep logger a dependency-free package?

Pros (current implementation):

  1. logger does not import any user-contributed packages, so 1) can be safely and quickly installed in restricted environments, 2) there is no chance that recursive dependencies bloat up suddenly, and 3) only bugs in base R packages can sneak into logger;
  2. Users who do not have glue installed or do not make glue the default formatter, very likely have a strong reason for this choice;
  3. Users who have glue installed get formatter_glue as the default formatter either way.

Cons:

  1. If someone does not have glue installed when using logger (very unlikely given the broad list of highly popular packages which import it), they must install it themselves to use formatter_glue;
  2. There are a few extra lines in the code which handle the fallback to formatter_sprintf if glue is not available, which can be a maintenance burden;
  3. There is a tiny overhead (~2.5µs) due to the call fail_on_missing_package("glue") which could be avoided by ensuring glue is always available.

If one really wants to help those users who are affected by Cons 1) , it is easy to create logger.glue which imports glue and logger. As for Cons 3), the overhead could be avoided by refactoring these checks. I am happy to create a PR if this is really needed, but I think the overhead is negligible to the total runtime of any log_info() etc. call.

@hadley
Copy link
Collaborator Author

hadley commented Aug 23, 2024

@tdeenes for content, this PR is a consequence of a live discssuion with @daroczig.

My primary concern with the current behaviour is that it will silently behave differently depending on whether or not glue is installed, and IMO that's a major drawback in a logging package which you would hope to behave identically in every possible scenario.

Additionally, glue is itself a 0-dependency package (and always will be) so this is an extremely lightweight dependency. And this is not a slippery slope — logger can take 1 dependency without any plans to take more in the future.

@tdeenes
Copy link
Contributor

tdeenes commented Aug 24, 2024

Yup, I understand this concern - even more so, I agree with it - but I still would let the users of the package decide what they want via env vars and options. So basically extend the already existing LOGGER_LOG_LEVEL with LOGGER_FORMATTER (actually, R_LOGGER_LOG_LEVEL and R_LOGGER_FORMATTER to avoid potential conflicts). The current behavior would correspond to "auto" or "glue_or_sprintf". We could even make the default layout and appender configurable this way.

I can create a PR tomorrow if there is interest.

@daroczig
Copy link
Owner

Thank you both!

Originally, I wanted to keep logger 0-dep, but using glue by default, except if it's not installed, is indeed messy, so I'm OK with properly depending on glue. This will be part of a planned major release (v1.0.0), so breaking changes are kind of expected anyway :)

Folks preferring the old behavior can pin logger to an older version -- or raise an issue here, and we can release a fork without glue (and potentially a bunch of helpers removed as well to provide an indeed minimal version).

Regarding the env vars: that's generally a good idea, but we should try to avoid using eval on any passed strings :) For LOGGER_LOG_LEVEL, that's solved, but for a flexible LOGGER_FORMATTER env var, that's less straightforward, as the formatter is an actual function - many cases returned by a generator fn.

I could imaging an env var to support the old behavior (using sprintf), but I'm not sure about opening this up to any formatter -- especially that the formatter fn is highly tied to code (e.g. R script or package), unlike the log level or layout/appender that should be configurable by the user of the code.

@tdeenes
Copy link
Contributor

tdeenes commented Aug 24, 2024

As for env vars: I have been thinking of a deliberately limited use case exactly for the reason that you mentioned. So only formatters, layout, and appender functions which are exported by the package would be supported. Something along the lines of:

.cfg_setting <- function(
    component,
    default,
    opt_name = .cfg_opt_name(component),
    env_var_name = .cfg_env_var_name(component),
    eval_env = .cfg_get_fn_factory(component)
) {
    component <- match.arg(
        component, choices = c("threshold", "formatter", "layout", "appender")
    )
    mget(ls())
}

.cfg_get_fn_factory <- function(component) {
    function(choice) {
        tryCatch(
            getExportedValue("logger", sprintf("%s_%s", component, choice)),
            error = function(e) {
                msg <- sprintf(
                    "'%s' is a not a valid choice for a '%s': %s",
                    choice, component, conditionMessage(e)
                )
                stop(msg, call. = FALSE)
            }
        )
    }
}

.cfg_opt_name <- function(x) {
    paste0("logger.", x)
}

.cfg_env_var_name <- function(x) {
    paste0("R_LOGGER_", toupper(x))
}

.cfg_get_default <- function(component) {
    component <- match.arg(component, choices = names(CONFIGURABLE))
    cfg <- CONFIGURABLE[[component]]
    ## attempt 1: try options
    opt <- getOption(cfg$opt_name, default = NULL)
    if (!is.null(opt)) return(opt)
    ## attempt 2: check env vars
    for (e in cfg$env_var_name) {
      env_value <- Sys.getenv(e, unset = NA_character_)
      if (!is.na(env_value) && nzchar(env_value)) {
        return(cfg$eval_env(env_value))
      }
    }
    ## last resort: return default
    cfg$eval_env(cfg$default)
}

CONFIGURABLE <- list(
    threshold = .cfg_setting(
        "threshold",
        default = "INFO",
        env_var_name = c("R_LOGGER_THRESHOLD", "LOGGER_LOG_LEVEL"),
        eval_env = as.loglevel
    ),
    formatter = .cfg_setting("formatter", default = "glue_or_sprintf"),
    layout = .cfg_setting("layout", default = "simple"),
    appender = .cfg_setting("appender", default = "console")
)

@tdeenes
Copy link
Contributor

tdeenes commented Aug 24, 2024

For backward compatibility we can keep the current logic and set the default for CONFIGURABLE$formatter based on the availability of glue, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants