Skip to content

Return from handle_message as to if message was handled or not #25

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions src/LoggingExtras.jl
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,7 @@ export TeeLogger, TransformerLogger, FileLogger,
ActiveFilteredLogger, EarlyFilteredLogger, MinLevelLogger


######
# Utilities for dealing with compositional loggers.
# Since the logging system itself will not engage its checks
# Once the first logger has started, any compositional logger needs to check
# before passing anything on.

# For checking child logger, need to check both `min_enabled_level` and `shouldlog`
function comp_shouldlog(logger, args...)
level = first(args)
min_enabled_level(logger) <= level && shouldlog(logger, args...)
end

# For checking if child logger will take the message you are sending
function comp_handle_message_check(logger, args...; kwargs...)
level, message, _module, group, id, file, line = args
return comp_shouldlog(logger, level, _module, group, id)
end
###############################
include("common.jl")

include("tee.jl")
include("transformer.jl")
Expand Down
4 changes: 3 additions & 1 deletion src/activefiltered.jl
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,11 @@ function handle_message(logger::ActiveFilteredLogger, args...; kwargs...)
log_args = handle_message_args(args...; kwargs...)
if comp_handle_message_check(logger.logger, args...; kwargs...)
if logger.filter(log_args)
handle_message(logger.logger, args...; kwargs...)
return handle_message(logger.logger, args...; kwargs...)
end
end
# otherwise
return MessageHandled(false)
end

function shouldlog(logger::ActiveFilteredLogger, args...)
Expand Down
36 changes: 36 additions & 0 deletions src/common.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@

"""
MessageHandled(::Bool)

`MessageHandled(false)` should be returned from a `handle_message` on a logger,
if it did not actually handle the log message.
For example, if the log message was below the level it should log.
This is of particular relevance to the [`ActiveFilteredLogger`](@ref), which can't know
util `handle_message` if a log message will be filtered or not.
Ideally, `MessageHandled(true)` would be returned from loggers when when they
successfully handled a message, however this is not strictly required.
Copy link
Member

@c42f c42f Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if a sink unsuccessfully handles a message, what then? Maybe most log routing networks should routinely be set up to have a safe-as-possible fallback of "just print synchronously to the console if all else fails"? Then there would be meaning for a sink returning MessageHandled(false)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Tangentially related thought: a composable safe fallback logger could allow us to move the try/catch logic which is currently hardcoded in the frontend out into a backend.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if a sink unsuccessfully handles a message, what then?

I imaging mostly that would result in an exception (e.g. could not write to file)
but that might not be a good thing.

(Tangentially related thought: a composable safe fallback logger could allow us to move the try/catch logic which is currently hardcoded in the frontend out into a backend.)

Something like #12

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

E.g Sinks should always return `MessageHandled(true)`.
"""
struct MessageHandled
val :: Bool
end

was_handled(response) = true # If we don;t get a MessageHandled then assume worked
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it makes sense to return missing here so that tee logger would use three-valued logic automatically. The caller can then distinguish "definitely handled", "maybe handled", "not handled." Example use-case may be that, in FirstMatchLogger #23:

  • !== false is considered true for info level and below
  • but only == true is considered true for more urgent messages (to make sure they are shown)

Not sure it's worth the complexity, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh interesting idea. Worth consideration

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or could it make sense to just return an Enum value? Then we have the option to expand the possible statuses later if necessary. On the downside making that forward compatible could be annoying.

If people always write utility routing tools in terms of was_handled (or other verbs we provide) forward compatibility would be easier. Can we somehow arrange was_handled to be the main API with the type MessageHandled as secondary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conversely: maybe was_handled should just be moved into TeeLogger
Right now it is only used to Booleanize the status so that can be written using handled |= was_handled(status)

was_handled(response::MessageHandled) = response.val


# Since the logging system itself will not engage its checks
# Once the first logger has started, any compositional logger needs to check
# before passing anything on.

# For checking child logger, need to check both `min_enabled_level` and `shouldlog`
function comp_shouldlog(logger, args...)
level = first(args)
min_enabled_level(logger) <= level && shouldlog(logger, args...)
end

# For checking if child logger will take the message you are sending
function comp_handle_message_check(logger, args...; kwargs...)
level, message, _module, group, id, file, line = args
return comp_shouldlog(logger, level, _module, group, id)
end
2 changes: 2 additions & 0 deletions src/earlyfiltered.jl
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ end
function handle_message(logger::EarlyFilteredLogger, args...; kwargs...)
if comp_handle_message_check(logger.logger, args...; kwargs...)
return handle_message(logger.logger, args...; kwargs...)
else
return MessageHandled(false)
end
end

Expand Down
1 change: 1 addition & 0 deletions src/filelogger.jl
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ end
function handle_message(filelogger::FileLogger, args...; kwargs...)
handle_message(filelogger.logger, args...; kwargs...)
filelogger.always_flush && flush(filelogger.logger.stream)
return MessageHandled(true)
end
shouldlog(filelogger::FileLogger, arg...) = true
min_enabled_level(filelogger::FileLogger) = BelowMinLevel
Expand Down
2 changes: 2 additions & 0 deletions src/minlevelfiltered.jl
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ end
function handle_message(logger::MinLevelLogger, args...; kwargs...)
if comp_handle_message_check(logger.logger, args...; kwargs...)
return handle_message(logger.logger, args...; kwargs...)
else
return MessageHandled(false)
end
end

Expand Down
5 changes: 4 additions & 1 deletion src/tee.jl
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@ function TeeLogger(loggers::Vararg{AbstractLogger})
end

function handle_message(demux::TeeLogger, args...; kwargs...)
handled = false
for logger in demux.loggers
if comp_handle_message_check(logger, args...; kwargs...)
handle_message(logger, args...; kwargs...)
resp = handle_message(logger, args...; kwargs...)
handled |= was_handled(resp)
end
end
return MessageHandled(handled)
end

function shouldlog(demux::TeeLogger, args...)
Expand Down
4 changes: 3 additions & 1 deletion src/transformer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ function handle_message(transformer::TransformerLogger, args...; kwargs...)
kwargs = new_log_args.kwargs

if comp_handle_message_check(transformer.logger, args...; kwargs...)
handle_message(transformer.logger, args...; kwargs...)
return handle_message(transformer.logger, args...; kwargs...)
else
return MessageHandled(false)
end
end

Expand Down