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

Documentation for the Logging stdlib #26005

Merged
merged 4 commits into from
Apr 9, 2018
Merged

Documentation for the Logging stdlib #26005

merged 4 commits into from
Apr 9, 2018

Conversation

c42f
Copy link
Member

@c42f c42f commented Feb 12, 2018

Here's some documentation covering the Logging stdlib.

Fixes #25154

@ararslan ararslan added docs This change adds or pertains to documentation logging The logging framework labels Feb 12, 2018
@c42f c42f mentioned this pull request Feb 13, 2018
enabled.

Second, the logging tools allow you to attach arbitrary data to each event as a
set of key value pairs. This allows you to capture local variables and other
Copy link
Member

@stevengj stevengj Feb 13, 2018

Choose a reason for hiding this comment

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

key=value pairs or key–value pairs

early return is more appropriate.)
* The *message* is a human readable string describing the event in markdown
format.
* Optional *key value pairs* allow arbitrary data to be attached to each event.
Copy link
Member

@stevengj stevengj Feb 13, 2018

Choose a reason for hiding this comment

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

key–value pairs

(note en dash)

[`AbstractLogger`](@ref).

When an event is triggered, the appropriate logger is found by looking for a
task local logger with the global logger as fallback. The idea here is that
Copy link
Member

@stevengj stevengj Feb 13, 2018

Choose a reason for hiding this comment

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

task-local logger (hyphenate compound adjectives)

(note hyphen, not an en dash)

task local logger with the global logger as fallback. The idea here is that
the application code knows how log events should be processed and exists
somewhere at the top of the call stack. So we should look up through the call
stack to discover the logger - that is, the logger should be *dynamically
Copy link
Member

@stevengj stevengj Feb 13, 2018

Choose a reason for hiding this comment

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

em dash (—), not hyphen, with no surrounding spaces

(I end up giving all my students the talk about hyphen vs en dash vs em dash vs minus sign. 😉 )

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I normally get the emdash correct, but there's no nice way to type it in markdown so I got sloppy ;-) Oddly enough, the julia markdown parser accepts the LaTeX -- for endash but not --- for emdash.

Copy link
Member

@stevengj stevengj Feb 14, 2018

Choose a reason for hiding this comment

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

On most operating systems there are keyboard shortcuts to type the Unicode dashes directly. For example, on MacOS you type option-hyphen for en dash and option-shift-hyphen for em dash.


Exceptions which occur while generating the log event are captured and logged
by default. This prevents individual broken events from crashing the
application which is helpful when enabling little-used debug events in a
Copy link
Member

Choose a reason for hiding this comment

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

comma before "which"

formatting and filtering. [`NullLogger`](@ref) is a convenient way to drop all
messages where necessary; it is the logging equivalent of the [`DevNull`](@ref)
stream. [`SimpleLogger`](@ref) is a very simplistic text formatting logger,
mainly useful for debugging the logging system itself.
Copy link
Member

Choose a reason for hiding this comment

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

What methods should a custom logger define?

Copy link
Member

Choose a reason for hiding this comment

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

If these are the ones in the Processing events section below, you can just cross-reference that here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a section about this.

* A `group` for the event which is set to the base name of the file by default,
without extension. This can be used to group messages into categories more
finely than the log level (for example, all deprecation warnings have group
`:depwarn`), or into logical groupings across or within modules.
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to attach additional information to log events?

For example, in IJulia we might want to attach additional information about the current execution cell, so that the log message is printed in the right output cell. (If necessary, we can extract this from the file metadata, since the input cell name is used as the filename, but I'm curious if there is a way to attach additional runtime information. For example, if you define a custom logger, could you implement a method to generate additional key-value pairs based on some dynamic global state at the time the log message is emitted?)

Copy link
Member Author

@c42f c42f Feb 14, 2018

Choose a reason for hiding this comment

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

Absolutely — dynamic context is something which can be added by installing a custom logger as a filter (add a key–value pair and pass to the next logger up the stack). This could include information such as:

  • Global or task-local variables as you're suggesting
  • The time of the event
  • A backtrace

I think this is going to be a common enough pattern that we could support it in the library. For now however it's pretty bare bones and provides just enough structure for this kind of stuff to be done outside Base/stdlib.

For IJulia I expect you'll want to install a custom logger anyway because formatting of log messages can be a lot nicer in the browser compared to the console.

Copy link
Contributor

@swt30 swt30 left a comment

Choose a reason for hiding this comment

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

At the risk of turning this into a proofreading session, I popped in some more minor grammatical/punctuation suggestions along the same lines as stevengj. I really love the new logging system, by the way---well done!


* The *log level* is a broad category for the message which is used for early
filtering. There are several standard levels of type [`LogLevel`](@ref);
user defined levels are also possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

user-defined

* The *log level* is a broad category for the message which is used for early
filtering. There are several standard levels of type [`LogLevel`](@ref);
user defined levels are also possible.
- Use `Debug` for verbose information which could be useful when debugging an
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use "that" instead of "which" here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, I've already removed most of these "which"s locally. Will push an update shortly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent, you've clearly got your pronouns under control then ;)


As you can see in the examples, logging statements make no mention of
where log events go or how they are processed. This is a key design feature
which makes the system composable and natural for concurrent use. It does this
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also use "that" instead of "which" here.


### Loggers

Processing of events is performed by a *logger* which is the first piece of
Copy link
Contributor

Choose a reason for hiding this comment

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

Comma before "which"

composing functionality from multiple modules.)

The global logger may be set with [`global_logger`](@ref), and task local
loggers controlled using [`with_logger`](@ref). Newly spawned tasks inherit
Copy link
Contributor

Choose a reason for hiding this comment

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

Newly-spawned

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, any gifts of compound adjective detection are gratefully received :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

task-local in the line above too!

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this is incorrect: you don't hyphenate compound adjectives where the modifier is an adverb ending with "ly": https://www.merriam-webster.com/words-at-play/6-common-hypercorrections-and-how-to-avoid-them/hyphenating-ly-adverbs

Copy link
Contributor

Choose a reason for hiding this comment

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

^ that's true, my bad!

somewhere at the top of the call stack. So we should look up through the call
stack to discover the logger - that is, the logger should be *dynamically
scoped*. (This is a point of contrast with logging frameworks where the
logger is *lexically scoped* - provided explicitly by the module author or a
Copy link
Contributor

@swt30 swt30 Feb 14, 2018

Choose a reason for hiding this comment

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

If you're fixing the hyphen -> em dash above then you should do this one too, or you could just turn it into a comma: "where the logger is lexically scoped, provided explicitly by the module author"

Copy link
Member Author

Choose a reason for hiding this comment

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

Already done (locally) :-)

@c42f
Copy link
Member Author

c42f commented Feb 14, 2018

I've pushed some tweaks to address the questions which came up here (thanks).

Writing the section about the AbstractLogger functions made me reconsider which functions should have default implementations. I think the defaults for shouldlog and min_enabled_level were more likely to cause confusion than be useful so I've removed those for now.

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

Love it! My only request would be maybe include a very quick "example AbstractLogger"? Maybe something like a "JSONLogger" that output everything in JSON? Just a thought.

@@ -169,9 +169,6 @@ overridden:

There's also some key value pairs which have conventional meaning:

* `progress=fraction` should be used to indicate progress through an
Copy link
Member

Choose a reason for hiding this comment

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

why take this out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I loved this feature and removing it makes me sad. But I think it's too unreliable as a user facing interface for progress reporting. Consider

for i=1:N
    foo()
    @info "blah" progress=i/N
end

This looks super simple and nice, but what if foo() throws? Then you're left with an incomplete progress bar which needs to be cleaned up with a heuristic of some kind.

We might "fix" this by limiting things to one progress bar at a time. But this doesn't make sense for nested progress bars and more general concurrent situations where the loop above may be executing from more than one task at once. In these cases you really want a reliable way to detect when the progress step is complete.

So I think it's better to remove this for now and reintroduce a more reliable progress reporting interface later.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a sane approach. Better to be a little cautious now and give the good stuff later.

composing functionality from multiple modules.)

The global logger may be set with [`global_logger`](@ref), and task-local
loggers controlled using [`with_logger`](@ref). Newly-spawned tasks inherit
Copy link
Member

Choose a reason for hiding this comment

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

Don't hyphenate compounds for an adverb ending in "ly" (google it)…

@KristofferC
Copy link
Member

Is there a way to just write text to a logger? Basically just want to use the stream of the logger but completely format it myself.

@c42f
Copy link
Member Author

c42f commented Feb 16, 2018

@KristofferC logging produces a stream of log events, each of which has the structure described in the documentation. You can encode formatting hints in this structure if you so please, but not all loggers will understand such hints unless there's a common convention.

Providing precise formatting control is problematic because it tightly couples the structure of events to the display capabilities of the logger which can vary widely depending on the environment (console vs juno vs ijulia for example). So you'd like to provide semantic structure in the message which can be interpreted according to the display capabilities which are present. My current best solution for rich formatting of log messages is to document that message strings should be encoded as markdown. This doesn't cover all cases (eg, color) but I think it's a practical compromise for now. I have a branch I was working on to make ConsoleLogger take markdown seriously, I just got a bit bogged down in refactoring Base.Markdown to make it actually work.

@stevengj
Copy link
Member

@c42f, do messages have to be strings? If they are opaque objects that simply have appropriate show methods (much like documentation objects), then in principle they could have any kind of rich formatting on appropriate output devices.

@c42f
Copy link
Member Author

c42f commented Feb 16, 2018

@stevengj there's no fundamental reason that messages need to be strings. I feel like having strings be interpreted as markdown is a nice default which is consistent with docstrings. But using show would probably work out nicely. Perhaps something like Logging.showmessage with the default implementations:

showmessage(io, mime, msg) = show(io, mime, msg)
showmessage(io, mime, msg::AbstractString) = show(io, mime, Markdown.parse(msg))

?

@stevengj
Copy link
Member

@c42f, I'm skeptical that you should define a showmessage function. I would think that:

  • If the caller wants to pass a markdown-formatted string, they should use md"...." to create a Markdown.MD object.

  • The current Logger be much like a display function: it should know what kinds of MIME types it can handle, call mimewritable(mime, x) to find out whether the message supports those MIME types, and then call show(io, mime, x) to output it (defaulting to text/plain if no other supported MIME types are found).

@maleadt
Copy link
Member

maleadt commented Feb 16, 2018

Cool, docs :-)

How would I define a custom log level? I tried:

import Base.CoreLogging: @_sourceinfo, logmsg_code, Debug
const Trace = Debug-1000
macro trace(message, exs...) logmsg_code((@_sourceinfo)..., :Trace, message, exs...) end

But that gives some hygiene errors on code returned by logmsg_code.
Or isn't this intended to be extensible yet? And maybe it makes sense to add a trace level to Base.

Also, the with_logger/global_logger approach seems application oriented. Any suggestions/guidelines for library developers? Say, if I replace logging in my package with the new logging macros, and a user encounters some error, how do I instruct him to ramp up the log level for my package (ie. not just replacing the global logger, which is also pretty user-unfriendly)?

@c42f
Copy link
Member Author

c42f commented Feb 17, 2018

If the caller wants to pass a markdown-formatted string, they should use md"...." to create a Markdown.MD object.

I tried this in an earlier prototype and I didn't care for it.

Just as for docstrings, I think the simplicity and consistency of having an opinionated default makes sense here. In this system the message is "for humans" and the structured key-value pairs is a way to pass auxiliary machine readable data. So having strings which support simple formatting by convention is a win in my opinion. If we were to advertise in the documentation that people can just put an md in front of the message if they feel like it, we'll probably just get less consistency and less readability across the ecosystem as a whole. Coming back to docstrings again, making them markdown was an opinionated design choice, but I think it's worked out really well and I think there's a close analogy here.

@stevengj
Copy link
Member

@c42f, in the documentation, the strings are passed through the markdown parser by default, but you can still pass any other kind of object that you want, and it uses the show machinery for rendering. You aren't prevented from using non-Markdown strings.

@c42f
Copy link
Member Author

c42f commented Mar 1, 2018

@stevengj yes, what I'd like to achieve is the same as the documentation system: pass strings through the markdown parser by default, and do "something sensible" with other types. How about we change

The message is a human readable string describing the event in markdown format.

to

The message is an object describing the event. By convention AbstractStrings passed as messages are assumed to be in markdown format. Other types will be displayed using show(io,mime,obj) according to the display capabilities of the installed logger.

@c42f
Copy link
Member Author

c42f commented Mar 1, 2018

@maleadt good questions, mostly the answer to those is "it's not implemented yet, but yes we should have a thing there". Perhaps I should just remove mention of custom log levels for now and we can make sure they work smoothly later. At the moment all you can do is define a custom level and use the generic @logmsg with that level (as in the tests).

I definitely agree that a @trace level is warranted. My feeling is that it should behave a little differently from the other logging macros because for tracing, efficient filtering based on a transparent label is critical. Perhaps something like:

@trace :event_label a=1 b=2

I guess the event label goes into the group variable for ease of filtering.

We should learn as much as possible from the solaris dtrace system which seems super well thought out http://dtrace.org/guide/preface.html#preface.

For now I'd like to punt on the details of tracing and get this merged!

c42f and others added 3 commits April 9, 2018 08:55
Remove default implementations for shouldlog and min_enabled_level as
these seem more likely to cause confusion than be useful.
@maleadt
Copy link
Member

maleadt commented Apr 9, 2018

Rebased and added a snippet on #26265.

Anybody opposed to merging this? All review comments seem addressed, and the remaining questions are on the design of the logging system. I don't think these should hold back documentation on code that has been merged anyway.

@KristofferC
Copy link
Member

+1 for merging, this is inf better than what we currently have :)

@stevengj
Copy link
Member

stevengj commented Apr 9, 2018

Would like to see the clarification/fix on requiring markdown suggested by @c42f above, but otherwise +1.

@maleadt maleadt merged commit b08c262 into master Apr 9, 2018
@maleadt maleadt deleted the cjf/logging-docs branch April 9, 2018 13:13
@c42f
Copy link
Member Author

c42f commented Apr 10, 2018

Thanks a heap @maleadt for following this up, and sorry for being absent from the tail end of this discussion. (I'm in a new job and feeling both distracted by that and also burned out in keeping up with many open source loose ends. Bleh :-/ )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation logging The logging framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants