Add LaunchLogger class

[jacobperron]

Supports logging messages from multiple processes and modules to the screen, a common log file, or both.
LaunchLogger is a singleton and when it is first instantiated a log file is created of the form 'DATETIME-launch-HOSTNAME-PID.log'.
Messages sent to the screen or the log file will have the format 'TIME [LEVEL] NAME: MESSAGE'.

Connects to #104

Tests were added that capture and check stdout, but I could only get them working by supplying pytest with the -s option. I'm not sure why this is required.
E.g. colcon test --pytest-args -s --packages-select launch

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

lgtm, other than some style comments.

[wjwwood]

Like where? Generally, I'd like to avoid indeterminate TODO's, at best they have a "do X when Y is possible/occurs" type structure. So I'd like to resolve this TODO before merging.

This seems like a reasonable place to initialize the logging, was there a specific use case you have in mind where this might not work out?

[wjwwood]

This isn't a great name for the decoration, imo. It's not making the function a singleton, it's adding attributes to the function. Maybe add_logging_attributed or just logging_attributes.

[wjwwood]

Please wrap all arguments or none, and do not vertically align like this. All indentation should be a multiple of 4 and should only indent one level at a time.

[wjwwood]

def launchConfig(
    *, level=None, log_dir=None, screen_format=None,
    log_format=None, screen_style=None, log_style=None
):

or

def launchConfig(
    *,
    level=None,
    log_dir=None,
    screen_format=None,
    log_format=None,
    screen_style=None,
    log_style=None
):

[wjwwood]

This parameter comes before the previously documented on (screen_style) in the function declaration, please document them in the order they appear in the function declaration.

[hidmic]

Running CI again:

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

@wjwwood ready for re-review and merge! :)

[wjwwood]

This lgtm, I went through and fixed some style issues, and some docs, and some style issues with the docs :D.

Also, I noticed the use of methodName rather than method_name, which is more common in launch itself. I guess you were trying to emulate the logging module from Python in some cases, to have launch.logging be a drop in replacement of sorts for it.

I'm not sure how I feel about that. I kind of prefer people to use Python's logging directly and the launch.logging module only be used for configuration and special cases. This is pretty much what's happening right now anyways, for instance the only place that launch.logging.getLogger() is being used instead of logging.getLogger appears to be in the launch service, even though there are numerous other cases throughout the code that get the logger directly from Python.

I opened a draft pr (#190) so we can discuss it, because I'd like to know what you think about.

Related to that, I can see the convenience of forwarding some of the logging module's contents via launch.logging, e.g. forwarding logging.DEBUG as launch.logging.DEBUG, but for most of the cases, it seems like having people just use logging directly might be better. Especially if the above draft pr is the direction we want to go in.

[hidmic]

I guess you were trying to emulate the logging module from Python in some cases, to have launch.logging be a drop in replacement of sorts for it.

That's correct. As a result of providing all necessary functionality (e.g. a reset() to prevent successive LaunchService instances from interacting with each other, not the same as shutdown() as this one can only be called once per process) and keeping the API simple (e.g. no need to configure each logger every time to output to screen and the launch log file) without preventing full use of the logging module capabilities. It seemed to me it was the most sane way to do it considering that logging state is global (e.g. having code affecting logging.root verbosity in launch.logging and user code seemed awkward).

An alternative, though it wouldn't allow for some of the functionality added now, would be to turn launch.logging into a set of functions to configure loggers. And leave everything else to be set through the logging module.

This is pretty much what's happening right now anyways, for instance the only place that launch.logging.getLogger() is being used instead of logging.getLogger appears to be in the launch service, even though there are numerous other cases throughout the code that get the logger directly from Python.

I'm under the impression that you're not considering the changes added by #147 (which we should probably merge into this one). Using launch.logging.getLogger() instead of logging.getLogger() is a convenience to avoid having to explicitly attach screen and launch log file handlers to each and every logger instance. And no, we're not leveraging loggers' hierarchy for common handling because I was unable to use hierarchy relationships to correctly model process output configurations.

[hidmic]

BTW thanks for the typing annotations :) Haven't had the time to setup mypy on Emacs yet.

[wjwwood]

I'm under the impression that you're not considering the changes added by #147 (which we should probably merge into this one).

Yup, that bit me again. I would support merging them.

So, you are using the custom getLogger in a few places, but I still think it's important for users to realize that this function is from launch and not python itself, so I stand by my request to make it launch.logging.getLogger everywhere in the other pr. Following that logic, I think the function doesn't need to look like the one from python's logging, and so I wouldn't change my proposal in #190, though could still be convinced to not go that way.

What do you want to do, leave it as is or make launch.logging conform to the rest of launch rather than emulate Python's logging module?

[hidmic]

Ok, I think I see where you're going. To have launch.logging taking care of it's own subtree of loggers, right? I'll take your draft and move things around a bit.

[hidmic]

@wjwwood Alright, done. I had to rebase to solve some merge conflicts.

There're two things I'm not very fond of though:

1. To have launch_config() affecting logging.root verbosity.
2. To have reset() affecting all loggers known to logging.

I'd be inclined to push all launch.logging loggers below a common namespace like launch.<logger-name> to scope the effect of the aforementioned functions. Thoughts?

[wjwwood]

I'm not sure why but there are still merge conflicts.

[hidmic]

Uh. That's odd. I'll rebase again.

[hidmic]

@wjwwood ping

[wjwwood]

lgtm

---

There're two things I'm not very fond of though:

• To have launch_config() affecting logging.root verbosity.

Yeah, I guess so, but couldn't you instead store the requested log level and apply it only to the loggers as they're created using launch.logging.* functions? Effectively preventing them from inheriting from the root logger's settings or you could ensure the given log level is not less than the inherited log level, so that setting the root logger to debug would still affect all launch loggers even if the log level given was info, for example.

• To have reset() affecting all loggers known to logging.

I don't understand the problem with this one.

I'd be inclined to push all launch.logging loggers below a common namespace like launch.<logger-name> to scope the effect of the aforementioned functions. Thoughts?

I'd prefer to not do that since it just makes all the names longer, though I could be convinced otherwise if there were example outputs which didn't look too bad. Instead I'd prefer to keep track of which "top level" loggers need to be affected without having a namespace for them all.

In the end, I think it won't matter that much as the default use case is running launch in it's own process by itself. The will be cases where that's not true, but I don't feel like this is going to be a big issue, but I could be underestimating it.

[hidmic]

Yeah, I guess so, but couldn't you instead store the requested log level and apply it only to the loggers as they're created using launch.logging.* functions?

Hmm, the problem with that approach is that launch_config() may be called more than once with different levels or even coexist with levels set on a per logger basis by the user. Having a root logger helps implementing the fallback mechanism (i.e. if no level is set here, use the parent's).

I don't understand the problem with this one.

That it's counter-intuitive if you're using the raw logging module too. A launch.logging.reset() will affect loggers created/retrieved by logging.getLogger(). It seems to my like the kind of caveat/feature/bug one may lose an hour on if the documentation was not read thoroughly.

In the end, I think it won't matter that much as the default use case is running launch in it's own process by itself.

That's a fair argument. Though we'll have a testing framework using launch as a module in short.

---

Anyways, I'll rebase, test and merge this to get things going. We can revisit it later (even open issues if you think it's worth it). Thanks for the review @wjwwood !

[hidmic]

Alright, CI passed. I'll merge this one, we can discuss outstanding issues/caveats in future PRs.