Change the behavior of excluded_exceptions option

[Jean85]

This PR stems/fixes #820 and the related getsentry/sentry-symfony#216.

The current behavior of the excluded_exceptions option is broken, since it just excludes exceptions from the exception key in the event payload, which results in broken or empty exception chains reported.

Instead, the correct behavior should be ignoring the automatic capturing of a set of exceptions, like excluding 404 exceptions from frameworks.

[stayallive]

👌

[ste93cry]

I was thinking if it makes sense to do the check of the option in the captureEvent method instead of in captureException (which is just a "proxy" method). The reason is that if someone uses the first one the option will not work, which may not be consistent with the expected behavior

[Jean85]

I considered that approach, but it's less clear: it would have to inspect the exception key of the payload, and it doesn't have a way to discern if that is the origin of the event, or if it's just a collateral.

IMHO the captureException conveys the correct requested behavior, and it's more controllable. If anyone wants to apply a blanket ignore on the exception key of the payload, then before_send is the correct way to go, and it's not something that we should touch ourselves.

[ste93cry]

it doesn't have a way to discern if that is the origin of the event, or if it's just a collateral

I suppose we should not care if the exception is originating the event or is just additional data of the payload as long as the option works coherently

If anyone wants to apply a blanket ignore on the exception key of the payload, then before_send is the correct way to go, and it's not something that we should touch ourselves

Setting the exception key of the payload is the official way to set an exception on the event object, so applying the option on it would just provide coherent behavior across all methods. I'm really torn on what to do here, maybe an opinion from @HazAT or @stayallive could help us take a decision. Apart from this point, LGTM

[stayallive]

Hmm this is a tricky one, but I would say captureException makes more sense. Since that deals with capturing an exception specifically.

If I were to call captureEvent myself I would not expect it to drop the event because I included an ignored exception. (possibly even nice to have the option to do submit ignored exceptions "manually")

[ste93cry]

If I were to call captureEvent myself I would not expect it to drop the event because I included an ignored exception

Wouldn't the same reasoning apply to the captureException method?

[stayallive]

Yes. Yes it would. But since its called excluded_exceptions this makes more sense IMO.

Best case would be to only exclude “auto” captured exceptions. But we have no way of identifying them currently so IMO this would be the best approach.

[ste93cry]

I'm not really happy with some options (this one being one of them) that apply only in certain situations since they make the behavior less predictable or not predictable at all. However, LGTM, let's see what happens and if someone else complains we will act accordingly 👍

[stayallive]

Probably a good thing to actually add this option to the documentation and be explicit about how it works there. Than at least it’s documented.

[ste93cry]

Yup, if someone can take care of opening a PR in the getsentry/sentry-docs repository it would be great!

[Jean85]

Done in getsentry/sentry-docs#1019. To be fair, the docs already described the behavior that this PR implements, I only added the captureEvent clarification.

[pgrimaud]

Hi,

A new release with this fix is ​​expected soon? 😄
I hate having package as dev-master to my composer.json files.

Thanks

[rvanlaak]

Could there be a regression introduced with the v2.2.x minor? getsentry/sentry-symfony#216 (comment)

[Jean85]

Nope, that's related to the Symfony bundle. Also, this MR is present and released with d407aa1 since 2.1.1.