thephpleague / tactician-logger

Adds PSR-3 logging support to the Tactician Command Bus
62 stars 8 forks source link

Feature/normalizer docs #7

Closed rosstuck closed 8 years ago

rosstuck commented 8 years ago

Before tagging a new release, I added a few docs, please let me know if you have any improvements, @tyx @boekkooi.

Also, I made one other change here that I'd like your feedback on: when looking at PSR-3, I noticed there's a clause in the context documentation about a standard "exception" key to hold original exception objects in, so that other storage utils can inspect the original error and collect the stack trace if they wanted.

That makes a lot of sense to me, but I was worried about clogging up file logs, so I hacked together a few tests. In general, Monolog outputs this format:

[2016-05-22 10:05:17] name.ERROR: Bar {"key":"value","exception":"[object] (LogicException(code: 0): battery empty at /home/rtuck/code/tactician/logger-test/test.php:18)"} []

This is a little more verbose but it doesn't include the entire stacktrace (unless you enable that in monolog). I'm not sure how other libraries handle Exception objects but I imagine they follow the spec reasonably well here, so I'd propose making this change before we tag an 0.10.

Let me know if you have any objections :)

boekkooi commented 8 years ago

Great catch on the exception context key! I added some minor remarks but it's looking good.

rosstuck commented 8 years ago

After looking at the interface again today, I had to admit I wasn't totally satisfied. Also, I recently got some other feedback that someone wanted to be able to change the log levels between critical and error depending the type of exception (something we don't easily support right now).

I was wondering how to support all this and I only really see the following options.

a) we create some kind of LogMessage VO that you can optionally return that holds the message, context and log level. b) we simplify the interface drastically. c) we deprecate this package since it's fairly trivial to do on your own.

I think A is over the top, C doesn't fit with our "save 30 minutes" plugin mentality, so what about this as B?

class SomeFormatter implements Formatter
{
    public function commandReceived(LoggerInterface $logger, $command)
    {
        $logger->debug('Command received: ' . get_class($command));
    }

    public function commandHandled(LoggerInterface $logger, $command)
    {
        $logger->debug('Command succeeded: ' . get_class($command));
    }

    public function commandFailed(LoggerInterface $logger, $command, Throwable $e)
    {
        $logger->error(
            'Command failed: ' . get_class($command)
            ['exception' => $e]
        );
    }
}

The only feature we loose is changing the log levels based off the constructor params to the middleware, but I don't honestly believe anyone uses it (they're likely better off just adjusting their log level or we can create a wrapper for this).

Sorry to dredge this up into a design discussion again!

tyx commented 8 years ago

This solution looks nice for me. πŸ‘

rosstuck commented 8 years ago

Okay, done here, lemme know what y'all think. It's basically a rewrite of the package. :dancer:

@boekkooi I'm not sure how to keep Throwable support in here right now since I can safely try to catch it in 5.6 but the formatter interface really needs a type definition and I can't typehint Throwable in 5.6. Any ideas/tricks? If y'all can give it a shake and are both happy I'm good with tagging this for release tomorrow.

rosstuck commented 8 years ago

I will take your silence as stern, tacit approval and will merge/tag this soon soon. :grin:

boekkooi commented 8 years ago

+1 sorry didn't see the notice before. Not having throwable is a bummer but a extra middleware for that can be added later.

-----Original Message----- From: "Ross Tuck" notifications@github.com Sent: β€Ž18/β€Ž08/β€Ž2016 08:01 To: "thephpleague/tactician-logger" tactician-logger@noreply.github.com Cc: "Warnar Boekkooi" warnar@boekkooi.net; "Mention" mention@noreply.github.com Subject: Re: [thephpleague/tactician-logger] Feature/normalizer docs (#7)

I will take your silence as stern, tacit approval and will merge/tag this soon soon. 😁 β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

rosstuck commented 8 years ago

No worries. If you've got any ideas, I'm totally open to changing it still. :)