invenia / Memento.jl

A flexible logging library for Julia
https://invenia.github.io/Memento.jl/latest
Other
87 stars 14 forks source link

Inherit logger.record from parent loggers and from root #138

Closed peteristhegreat closed 4 years ago

peteristhegreat commented 4 years ago

I am following the conclusion.md example, and having to call setrecord!() after each call to getlogger seems like it isn't hierarchical. I propose this change in loggers.jl:

function getlogger(name="root")
    logger_name = name == "" ? "root" : name

    if !(haskey(_loggers, logger_name))
        logger = Logger(logger_name)
        register(logger)
        # inherit record type from root and parents
        ancestors = []
        if name != "root"
            push!(ancestors, "root")
        end
        hierarchy = split(name, ".")
        for i in length(hierarchy)-1:-1:1
            push!(ancestors, join(hierarchy[1:i], "."))
        end
        @show ancestors
        for ancestor in ancestors
            if name != ancestor && haskey(_loggers, ancestor) && _loggers[ancestor].record != DefaultRecord
                logger.record = _loggers[ancestor].record
                break
            end
        end
    end

    return _loggers[logger_name]
end

Then it would have a cascading effect of any loggers registered after a root or parent logger is set up, would inherit the record type specified.

This may have some ordering issues if a root logger is setup after a more specific logger is made. Maybe a root or parent logger, upon registering should push this setting down to its children that are still on default.

Does this fit in with the design of Memento? Should I make a pull request?

rofinn commented 4 years ago

Hmmm, yeah, I think this would fit with the overall design workflow. I might need to play with a final prototype, but that seems like a mostly reasonable solution. I'll note that a simpler solution might be to just add a recursive flag to setrecord! which should be as simple as:

function setrecord!(logger, rec; recursive=false)
    logger.record = rec
    recursive && setrecord!.(getchildren(logger.name), rec; recursive=recursive)
end

This would be a bit more explicit and wouldn't change the default behaviour while also making it easy to change the default record type through all or part of the tree.

peteristhegreat commented 4 years ago

But would that still require a strict ordering of when you call setrecord! to be set after the children loggers are registered, otherwise the children loggers won't pick up the changed record?

rofinn commented 4 years ago

That's true, but generally you should be loading all the loggers (by loading the packages that register them) first and then performing any application level configuration last (e.g., adding handlers, setting log levels, setting the record type).