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

Add the IgnoreErrorsIntegration integration and deprecate the excluded_exceptions option #928

Merged

Conversation

ste93cry
Copy link
Collaborator

@ste93cry ste93cry commented Dec 6, 2019

Q A
Branch? 2.3
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
License MIT

Closes #924. I decided that I didn't want to use the $payload argument of the capture* methods as an hacky way to pass options that configured the behavior of the method. Instead, like in the JS SDK I added a new integration that can be further expanded in the future and that will centralize all the logic to decide whether an event should be captured or not according to a set of criteria that must match with the event data. Being an integration means that the configuration now applies to all those methods and not only to the captureException one, so we cannot enable it by default until 3.0

@ste93cry ste93cry added this to the 2.3 milestone Dec 6, 2019
@ste93cry ste93cry force-pushed the feature/add-inbound-filters-integration branch 4 times, most recently from f8209f1 to f3c40e4 Compare December 8, 2019 22:54
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, apart from a small nitpick

src/Integration/InboundFiltersIntegration.php Outdated Show resolved Hide resolved
@ste93cry
Copy link
Collaborator Author

ste93cry commented Dec 9, 2019

I see that the Go SDK has a similar extension named IgnoreErrorsIntegration and I wonder if I should use the same name as it seems more appropriate and also clearer. Any opinion?

@Jean85
Copy link
Collaborator

Jean85 commented Dec 9, 2019

👍 for that name, I like it

@ste93cry ste93cry force-pushed the feature/add-inbound-filters-integration branch 2 times, most recently from e95cbb3 to 9f1dc2b Compare December 9, 2019 22:52
@ste93cry ste93cry changed the title Add the InboundFiltersIntegration integration and deprecate the excluded_exceptions option Add the IgnoreErrorsIntegration integration and deprecate the excluded_exceptions option Dec 9, 2019
Jean85 added a commit that referenced this pull request Dec 10, 2019
Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ste93cry ste93cry force-pushed the feature/add-inbound-filters-integration branch from 9f1dc2b to 4dadf66 Compare December 11, 2019 22:51
@ste93cry ste93cry merged commit 3b09f81 into getsentry:develop Dec 11, 2019
@ste93cry ste93cry deleted the feature/add-inbound-filters-integration branch December 11, 2019 23:02
@VincentLanglet
Copy link
Contributor

So we'll have to rename sentry.options.excluded_exceptions by sentry.options.ignore_exceptions ?

@ste93cry
Copy link
Collaborator Author

ste93cry commented Dec 15, 2019

No, you can leave your code as is and it will keep working until 3.0. However, the excluded_exceptions option has been deprecated in favour of enabling the IgnoreErrorsIntegration integration that will be applied every time an event is captured, so it will work also when the Monolog handler is involved

@VincentLanglet
Copy link
Contributor

so it will work also when the Monolog handler is involved

The excluded_Exceptions wasn't working for the Monolog handler (getsentry/sentry-symfony#273)

As soon as the 2.3 version is released, which of the following sentences is true ?

  • The excluded_exceptions will work also for the Monolog handler
  • We'll have to enabling the IgnoreErrorsIntegration if we want to ignore exceptions for the Monolog handler (But how ? With a ignore_exceptions ?)
  • Some development is necessary for sentry-symfony in order to handle the IgnoreErrorsIntegration.

Thanks !

@ste93cry
Copy link
Collaborator Author

The excluded_Exceptions wasn't working for the Monolog handler

This is the reason of this PR, to make such option work in all cases regardless of who is calling the Client::capture* methods. To do it, an integration has been created and once enabled it will affect all those methods (instead the excluded_exceptions was not affecting the Client::captureEvent method that was used by the Monolog handler): this is the reason we cannot enable it out-of-the-box until 3.0 or we risk of breaking existing applications. So, at the end of the story:

  • The excluded_exceptions option is deprecated as of version 2.3 and will be removed in 3.0
  • The IgnoreErrorsIntegration will take the place of the old option. To use it you will need to use the integrations option to instantiate the class and pass to its constructor the values that you did pass to the excluded_exceptions. You can then stop using the deprecated option and everything should work as before except that this new integration will affect all the Client::capture* methods (and not only the captureError and captureException) and consequently the Monolog handler
Sentry\init([
-    excluded_exceptions => [
-        Exception::class,
-    ],
+    integrations => [
+        new Sentry\Integration\IgnoreErrorsIntegration([
+            Exception::class,
+        ]),
+    ],
]);

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

Successfully merging this pull request may close these issues.

None yet

4 participants