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

Don't log nothing. #27650

Closed
wants to merge 2 commits into from
Closed

Don't log nothing. #27650

wants to merge 2 commits into from

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Jun 19, 2018

The logging macros support a very useful block-like syntax where the body builds and returns the message to log, making it easy to conditionalize complex or expensive message building code:

julia> @info begin
           msg = "expensive log message"
           msg
       end
[ Info: expensive log message

Meanwhile, some C APIs have additional log buffer or log level arguments, and as such need some log-conditional set-up that does not immediately produce a message. One solution would be for the logging macros to ignore nothing messages, enabling the following paradigm (taken from CUDAdrv.jl):

@debug begin
    options[INFO_LOG_BUFFER] = Vector{UInt8}(undef, 1024*1024)
    options[LOG_VERBOSE] = true
    nothing
end
optionKeys, optionVals = encode(options)

@apicall(:cuModuleLoadDataEx,
         (Ptr{CuModule_t}, Ptr{Cchar}, Cuint, Ptr{CUjit_option}, Ptr{Ptr{Cvoid}}),
         handle_ref, data, length(optionKeys), optionKeys, optionVals)

@debug begin
    options = decode(optionKeys, optionVals)
    if isempty(options[INFO_LOG_BUFFER])
        """JIT info log is empty"""
    else
        """JIT info log:
           $(options[INFO_LOG_BUFFER])"""
    end
end

An alternative solution would be to query the log level of the active logger, but that could be complex (which logger, env flag overrides, ...). This seemed like a DRY solution. Thoughts?

@maleadt maleadt added the logging The logging framework label Jun 19, 2018
@StefanKarpinski
Copy link
Member

An alternative would be having a @debug_setup macro or similar for this.

@maleadt
Copy link
Member Author

maleadt commented Jun 20, 2018

Yeah sure, I wanted to keep the API simple and nothing seemed like a good fit, but if deemed too creative I'm fine with _setup versions. I'll add some tests etc later today.

@yurivish
Copy link
Contributor

#27352 is possibly related, discussing the printing of nothing and missing when interpolated into strings.

@maleadt maleadt changed the title RFC: Don't log nothing. Don't log nothing. Jun 22, 2018
@maleadt maleadt mentioned this pull request Oct 16, 2018
@c42f
Copy link
Member

c42f commented Jul 16, 2019

Sorry I haven't been responsive on this — I generally don't have time to keep up with all the activity in JuliaLang/julia so I often miss things. You can ping me though ;-)

Does something like the alternative over at #29672 make sense? I think we could probably factor apart the early log checks so that they're usable independently of the log sinking part of the interface.

@maleadt
Copy link
Member Author

maleadt commented Jul 16, 2019

Does something like the alternative over at #29672 make sense? I think we could probably factor apart the early log checks so that they're usable independently of the log sinking part of the interface.

Yeah sure, the @debug nothing approach here seemed like a clean solution, but a proper @logif is fine for me as well. As long as I can get rid of these useless log messages 🙂

@vtjnash vtjnash closed this Apr 9, 2021
@vtjnash vtjnash deleted the tb/log_nothing branch April 9, 2021 22:54
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

This is stale, but still seems reasonable to me, if you want to revive it (or perhaps "missing" would be the more canonical?)

My opinion would be to require !(msg === nothing || isempty(kwargs)) to skip generating a log entry

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

Successfully merging this pull request may close these issues.

5 participants