Feature/filter exceptions

DayS commented 4 years ago

Goal

Provide a way to filter out some exception from report according to project-specific needs.

Design

This approach uses a simple Strategy pattern to handle all possible cases with little cost. A default implementation is provided to keep the current behavior : report all exceptions.

Changeset

Added

ErrorNotifyFilter
DefaultErrorNotifyFilter

Changed

BugsnagListener & Configuration
services.yml

Tests

Existing tests was updated to fit current behavior. A new test was added to test the new behavior.

Linked issues

This is related to #67 but with a higher view.

Review

For the submitter, initial self-review:

[x] Commented on code changes inline explain the reasoning behind the approach
[x] Reviewed the test cases added for completeness and possible points for discussion
[x] Check the scope of the changeset - is everything in the diff required for the pull request?
[x] A changelog entry was added for the goal of this pull request
This pull request is ready for:
- [x] Initial review of the intended approach, not yet feature complete
- [x] Structural review of the classes, functions, and properties modified
- [ ] Final review

For the pull request reviewer(s), this changeset has been reviewed for:

[ ] Consistency across platforms for structures or concepts added or modified
[ ] Consistency between the changeset and the goal stated above
[ ] Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
[ ] Usage friction - is the proposed change in usage cumbersome or complicated?
[ ] Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
[ ] Concurrency concerns - if components are accessed asynchronously, what issues will arise
[ ] Thoroughness of added tests and any missing edge cases
[ ] Idiomatic use of the language

tomlongridge commented 4 years ago

@DayS - thanks for your contribution.

However, I believe this is similar functionality as is provided in callbacks. Is there any reason why this wouldn't work for you?

DayS commented 4 years ago

Oh. It is, actually, quite similar. I didn't find this existing behavior before as it's quite hidden on the documentation IMYO. Also Callbacks seems to be more associated to side-effect implementation instead of an executor/strategy which can modify workflow execution. Thus, I didn't though about this as a possible solution :)

However, this option seems to work on my specific use-case (I use logic-specific Exception which are handled by a Normalizer to properly format them according to Rest API spec. I don't want them to be reported on Bugsnag), even if I was hopping to have the original Exception instead of just his name.

DayS commented 4 years ago

A little complement note on this, just in case someone needs this.

In fact Callback is not working for my use-case as I want to :

declassify my logic Exceptions from unhandled to handled, to prevent impacting stability score (I added in listener to consider each request as a session, so it acts as a Crash-Free Request)
ignore 4xx HttpExceptions

However, using a Middleware did the trick :

class ErrorFilterMiddleware
{
    public function __invoke(Report $report, callable $next)
    {
        // Ignore HTTP exceptions
        if (is_a($report->getName(), ClientErrorException::class, true)) {
            return;
        }

        // Override unhandled flag for logic exceptions
        if (is_a($report->getName(), AppException::class, true)) {
            $report->setUnhandled(false);
        }

        $next($report);
    }
}

Register from a custom client factory :

class ExtendedClientFactory extends ClientFactory
{
    public function make()
    {
        return parent::make()->registerMiddleware(new ErrorFilterMiddleware());
    }
}

Still, the only drawback at this point is to not having the actual Exception but it should do the job :)

So, you may either : close this PR, or ask me to update it to fix the compatibility with PHP 5.x (cf Travis job)

EDIT : Aaand I should sleep more. ClientErrorException is a no-go. We have to use HttpException, but at this point we have no way to know the http status code which came with the exception. The only way is override the whole BugsnagListener to dynamically inject this info in report's metadata in case of a HttpException.

tomlongridge commented 4 years ago

@DayS - currently amending the handled/unhandled state is a deliberate limitation due to the way sessions are reported. We would not recommend this approach without amending the session payload too.

This is a feature on our roadmap but it would probably be implemented on the bugsnag-php level.

I'll close this PR as callbacks should work for other fields in the report.

bugsnag / bugsnag-symfony