[BUG] Enhanced exception handling in Laravel causes false positives since 0.65.1

[sserbin]

Bug description

Enhanced exception handling for Laravel (#1322) seem to have introduced false positive error traces.

https://github.com/DataDog/dd-trace-php/pull/1322/files#diff-65a7a50856c71af24f66a705d37f039a87410525d7e102df692ad7cc110f494fR198 hooks into Kernel#renderException() which however is invoked before application's ExceptionHandler which then determines if the exception is worth reporting - see https://github.com/laravel/framework/blob/8.x/src/Illuminate/Foundation/Exceptions/Handler.php#L223

This means that exceptions like built-in exceptions AuthenticationException etc (as well as your custom exception types that you don't perceive as actual errors) are now being marked as error traces in apm, whereis previously they weren't. Doesn't seem like this is a useful/desired behavior for end-users.

PHP version

8.0.8

Diagnostics and configuration

Upgrading info

Upgraded from 0.64 to 0.66.0

[morrisonlevi]

Thanks for your report. Basically, you are saying we should only report exceptions which make it to this line: https://github.com/laravel/framework/blob/7acb44de81324d9577b0e161dad38ed6d3ee534d/src/Illuminate/Foundation/Exceptions/Handler.php#L247. Is that correct?

[sserbin]

Pretty much yeah. I'm not sure how you'd tap into that though - if you did have access to laravel app object, you can get ExceptionHandler from app/container and test for ExceptionHandler#shouldReport($e) before marking the root span as error.

[morrisonlevi]

I've been working on this task. I'm a bit conflicted on the exceptions in the list internalDontReport. For instance, if there was an AuthenticationException, it's a json response, and I'm looking at the trace, wouldn't I want that stack trace and error? This would be true for any exception which leads to a 4xx or 5xx, whether it's on the list of internalDontReport exceptions or not, I think?

Can you give us an example of a custom exception and what HTTP status it leads to? Most importantly, an example of one that leads to a 2xx or 3xx code so it wouldn't be an "error".

[sserbin]

For instance, if there was an AuthenticationException, it's a json response, and I'm looking at the trace, wouldn't I want that stack trace and error?

I don't personally think that's any useful in this particular case - AuthenticationException would usually be thrown when accessing auth-protected route and you're either not authenticated or your session has expired. In this case when I'm looking at the trace (for whatever reason) and I see it's 401 and I know what's happening. Besides the stack trace would often (if not always) be the same (bar the controller/route)

This would be true for any exception which leads to a 4xx or 5xx

5xx are a different story (but you normally wouldn't have an exception type on dontReport list and return 5xx), but for 4xx I think they won't be much useful stack traces in practice (to give a few examples of 4xx: AuthenticationException, AuthorizationExcetpion, ValidationException).

On the other hand, if they are included in the dd trace I guess I wouldn't mind them being there, as long as the root span isn't marked as an error in this case. The problem with having the root span as an error is that you then can't differentiate between a real error and the normal one (e.g. 'auth failed' error) when monitoring for app errors (either with automatic monitors in dd or just manually looking over the traces with 'error' filter)

Can you give us an example of a custom exception and what HTTP status it leads to? Most importantly, an example of one that leads to a 2xx or 3xx code so it wouldn't be an "error".

Something like an OptimistickLockException that leads to 409. Can't think of any example that would lead to 2xx or 3xx

[dshafik]

We actually have a case where we have an exception that leads to a 2xx. In this case it's for webhook handlers, where the request is for an object we don't know anything about, leading to a ModelNotFoundException but we catch it in the handler and return a 200 regardless, otherwise the client will retry.

Interestingly, this only seems to be an issue in PHP 8 for us, in PHP 7.3 we don't have this problem. Not sure if that's due to changes in Laravel or not…