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

[8.x] Improve exception logging #35722

Closed
wants to merge 3 commits into from

Conversation

levacic
Copy link
Contributor

@levacic levacic commented Dec 26, 2020

I'm not sure if this should go to 8.x or master, let me know if I should switch the target branch.

Here's the commit message from the second commit (the first is just an aesthetic change to make the second commit more readable - feel free to squash if needed):

Add special handling when logging exceptions

Previously, logged exceptions would be passed down to Illuminate\Monolog\Logger which forcibly casts them to strings, resulting in the __toString() method being called on them - which is inherited from Exception, which prints a stack trace. This stack trace ends up being used as the log message itself.

This change makes it so that when an exception is logged, the log message becomes a shorter and usually more readable string, which is the return value of the getMessage() method on the exception object, and passes the exception object itself as the exception key of the log message context, but only if that key wasn't already manually set by the user when logging the exception.

Monolog's LineFormatter will already process exceptions found in the context to include a nice stack trace, if the includeStacktraces option is turned on (which it is by default in Laravel).

This means that no information should be lost, unless someone is explicitly relying on the current behavior and/or making uncommon logging configuration changes.

Furthermore, this enables userland code to attach additional processors to the wrapped Monolog instance and include additional custom processing on the logged exception objects if desired.

Ref laravel/ideas#2449 (comment)

No semantic changes in this commit, it's just a style change to make the next
commit's diff easier to read, and make the final code more consistent.
-
[`Illuminate\Foundation\Exceptions\Handler::report()`](https://github.com/laravel/framework/blob/08303c7cbaa0b3271060ce315160d0722b2a78f0/src/Illuminate/Foundation/Exceptions/Handler.php#L233-L240)
already does this when logging caught exceptions
- This is well-defined behavior as per
[PSR-3](https://www.php-fig.org/psr/psr-3/#13-context)

Previously, logged exceptions would be passed down to
`Illuminate\Monolog\Logger` which forcibly casts them to strings, resulting in
the `__toString()` method being called on them - which is inherited from
[Exception](https://www.php.net/manual/en/exception.tostring.php), which prints
a stack trace. This stack trace ends up being used as the log message itself.

This change makes it so that when an exception is logged, the log message
becomes a shorter and usually more readable string, which is the return value of
the `getMessage()` method on the exception object, and passes the exception
object itself as the `exception` key of the log message context, but only if
that key wasn't already manually set by the user when logging the exception.

Monolog's
[`LineFormatter`](https://github.com/Seldaek/monolog/blob/78bd7bd33313c3a7ad1f2b0fc0c11a203d4e3826/src/Monolog/Formatter/LineFormatter.php#L168-L195)
will already process exceptions found in the context to include a nice stack
trace, if the `includeStacktraces` option is turned on (which it is by default
in Laravel).

This means that no information should be lost, unless someone is explicitly
relying on the current behavior and/or making uncommon logging configuration
changes.

Furthermore, this enables userland code to attach additional processors to the
wrapped Monolog instance and include additional custom processing on the logged
exception objects if desired.

Ref laravel/ideas#2449 (comment)
@levacic levacic changed the title [8.x] Improved exception logging [8.x] Improve exception logging Dec 26, 2020
@levacic
Copy link
Contributor Author

levacic commented Dec 26, 2020

PSR-3 also specifies it's okay to pass objects as messages as long as they implement the __toString() method (which all exceptions do), and that implementors may have special handling for such messages, so this isn't unreasonable as a use-case which could be supported.

@taylorotwell
Copy link
Member

We wouldn't change this on a patch release. Also would need to see before and after examples.

@levacic
Copy link
Contributor Author

levacic commented Dec 26, 2020

@taylorotwell Yeah wasn't sure how much of a BC break this would've been considered.

By before and after examples, do you just mean e.g. what gets output into laravel.log?

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

Successfully merging this pull request may close these issues.

2 participants