Grinnz / Mojolicious-Plugin-Log-Any

Mojolicious::Plugin::Log::Any - Use other loggers in a Mojolicious application
https://metacpan.org/pod/Mojolicious::Plugin::Log::Any
Other
0 stars 1 forks source link

Plugin enforces Mojo::Log short format #2

Closed jrubinator closed 4 years ago

jrubinator commented 4 years ago

There are several places in where we manually do

      $do_log = sub {
        my ($self, $level, @msg) = @_;
        my $formatted = "[$level] " . join "\n", @msg;

which is the equivalent to setting short in Mojo::Log.

The standard practice for Log::Any seems to be to have the Adapters do the formatting for you. I would expect Mojolicious::Plugin::Log::Any not to mess with the message at all before sending it to the adapter.

At the very least, by rewriting to

        my ($self, $level, @msg) = @_;
        my $formatted = $self->format(time, $level, @msg);

we would increase Mojo::Log compatibility and provide a means of turning off formatting (by allowing users to set format to a no-op).

Grinnz commented 4 years ago

That is done because of the lack of "level" or multi-line handling of all of the adapters. It is expected that the adapter will control any formatting (just like handling timestamps when needed and filtering by level).

Grinnz commented 4 years ago

In other words, it's by design that the Mojo::Log instance's "short" and "format" attributes are ignored.

jrubinator commented 4 years ago

I guess otherwise worded, my concern is that the following two log lines look different:

use Log::Any qw($log);

...
$app->log->error($message);
$log->error($message);

My gut was that they should be the same, since they're both going through Log::Any, but the former has the level prepended. That ends up looking funky if the adapter also prepends the level, eg. I wind up with

[error]: [error] My Test Message [error]: My Test Message

Grinnz commented 4 years ago

Out of curiosity what adapter are you using that prepends the level? I haven't come across any other than Mojo::Log.

Grinnz commented 4 years ago

One possibility I can think of is to add an attribute to the role to allow disabling its notation of the log level.

jrubinator commented 4 years ago

Out of curiosity what adapter are you using that prepends the level? I haven't come across any other than Mojo::Log.

It's a bit of a simplification of a homegrown one. There are also places in that Adapter that actively don't want the severity in the message.

One possibility I can think of is to add an attribute to the role to allow disabling its notation of the log level.

I had that thought too, and then thought that I was starting down the road to reinventing format.

Grinnz commented 4 years ago

This isn't really a formatting thing, just a workaround for the lack of the feature in most adapters, and thus disabling that workaround - I don't plan on adding any further option to format at this level.

jrubinator commented 4 years ago

a workaround for the lack of the feature

"the feature" being the prepending of the log level?

Grinnz commented 4 years ago

Yes. Specifically, the line you mention is the workaround.

jrubinator commented 4 years ago

Cool. If I submit a patch, are you okay adding the attribute to pass everything through untouched?

So

 if ($attr) {
    $do_log = sub {
      my ($self, $level, @msg) = @_;
      $logger->$level(@msg);
    };
}
Grinnz commented 4 years ago

No need - it will require a newer Mojolicious to use role attributes so I will add it shortly.

jrubinator commented 4 years ago

Thanks! Also thanks for explaining/being so responsive :)

Grinnz commented 4 years ago

I decided an attribute is not needed, an option to the plugin and attach_logger is sufficient. Released as v1.0.0. Thanks for the request.