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

What should we do about catch_errors for sinks? #38

Open
oxinabox opened this issue Jan 15, 2021 · 2 comments
Open

What should we do about catch_errors for sinks? #38

oxinabox opened this issue Jan 15, 2021 · 2 comments

Comments

@oxinabox
Copy link
Member

SimpleLogger (and thus FileLogger and DateRotatingLogger) set it to false.
ConsoleLogger sets it to true
FormatLogger sets it to true

Originally posted by @fredrikekre in #35 (comment)

@fredrikekre

Did you have any thoughts on this? Probably don't matter much, only captures message construction errors and those should not happen often.

@oxinabox:

Yeah I am not sure. It seemed fine to match ConsoleLogger.
On the onehand it is hard to debug exceptions that are not thrown
On the other hand your logger failing should not take down your process.

Maybe we should solve it compositionally.
Make all sinks set it to false,
then add a LoggingExceptionCatchingLogger that wrapps a logger and does catch_exceptions.

@fredrikekre

Yea that could be useful for also catching errors in handle_message (xref https://julialang.zulipchat.com/#narrow/stream/236331-logging/topic/Logger.20errors/near/222839637)

@oxinabox oxinabox mentioned this issue Jan 15, 2021
@fredrikekre
Copy link
Member

The current behaviour/meaning of catch_exceptions is just about whether to pass a log message or throw if the message itself can not be constructed. It feels like loggers where catch_exceptions(logger) == true should also swallow exceptions that comes from actually delivering the message (handle_message). But until that changes in Base there could be a SafetyLogger or FailSafeLogger that just puts try around everything. Quick draft:

struct SafetyLogger{L <: AbstractLogger} <: AbstractLogger                                   
    logger::L                                                                                
end                                                                                          
function handle_message(logger::SafetyLogger, args...; kwargs...)                            
    try                                                                                      
        handle_message(logger.logger, args...; kwargs...)                                    
    catch err                                                                                
        try                                                                                  
            println(stderr, "failed so print err and backtrace to stderr")                                                        
        catch err                                                                            
        end                                                                                  
    end                                                                                      
end                                                                                          
shouldlog(logger::SafetyLogger, args...) = shouldlog(logger.logger, args...)                 
min_enabled_level(logger::SafetyLogger) = min_enabled_level(logger.logger)                   
catch_exceptions(logger::SafetyLogger) = true

@oxinabox
Copy link
Member Author

oxinabox commented Feb 2, 2023

Leaning towards saying it should generally be set to true to match ConsoleLogger that's what people are most familar with.

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

No branches or pull requests

2 participants