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

[4.x] Rollback to is_a() to ignore exceptions instead of in_array() #1587

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

fmata
Copy link
Contributor

@fmata fmata commented Sep 13, 2023

Hello,

#1503 reintroduced ignore_exceptions in config and deprecated IgnoreErrorsIntegration. As reported in #1503 (comment) the behavior changed : before we could list interfaces or class hierarchy to ignore, after we must list all children exceptions (dozens or more in a decent project as stated by @stayallive).

It has been merged and released in the minor release 3.17. But deprecates a feature whithout upgrade path to have the exact
same feature in a minor release is not sem ver compliant.

Can you merge my PR and release a new patch version to rollback as before 3.17 ?

Thanks

@cleptric
Copy link
Member

While I agree that is_a might be beneficial, making these changes now can have worse effects on people who already use the new option. Hence, I'm not moving forward with this for now.

We can address this in the next major version of the SDK, which we already started working on.

@cleptric cleptric closed this Sep 13, 2023
@fmata
Copy link
Contributor Author

fmata commented Sep 13, 2023

@cleptric beneficial or not, it was the behavior until a minor release (3.17) which changed it without warning or upgrade note. The mistake is here, 3.17 already breaks many apps by alerting with false positives. Revert a breaking change of a documented feature is sem-ver compliant.

Mistake can happens :) but do not respect sem-ver is a bad signal for your users.

Or maybe there is a solution not deprecated. Can you give me an hint ?

@cleptric
Copy link
Member

Can you clarify what you mean by breaks many apps by alerting with false positives?
I also don't see any breaking change introduced in 3.17. The IgnoreErrorsIntegration works exactly as to prior versions. Hence, I find the claim us not following semver a bit exaggerated.

@fmata
Copy link
Contributor Author

fmata commented Sep 13, 2023

Using sentry/sentry-symfony,

Before 3.17 :

sentry:
    dsn: '%env(SENTRY_DSN)%'
    options:
        environment: '%app.deploy_env%'
        integrations:
            - Sentry\Integration\IgnoreErrorsIntegration
        send_default_pii: true
    register_error_listener: false
    register_error_handler: false

monolog:
    handlers:
        sentry:
            type: sentry
            level: !php/const Monolog\Logger::ERROR
            hub_id: Sentry\State\HubInterface

services:
    Sentry\Integration\IgnoreErrorsIntegration:
        arguments:
            $options:
                ignore_exceptions:
                    - ApiPlatform\Symfony\Validator\Exception\ValidationException
                    - Symfony\Component\HttpKernel\Exception\HttpExceptionInterface
                    - Symfony\Component\Security\Core\Exception\AccessDeniedException

All children of HttpExceptionInterface (NotFoundHttpException, BadRequestHttpException and so on...) are ignored.

After upgrade to >= 3.17, we removed all deprecated calls :

sentry:
    dsn: '%env(SENTRY_DSN)%'
    options:
        environment: '%app.deploy_env%'
        ignore_exceptions:
            - ApiPlatform\Symfony\Validator\Exception\ValidationException
            - Symfony\Component\HttpKernel\Exception\HttpExceptionInterface
            - Symfony\Component\Security\Core\Exception\AccessDeniedException
        send_default_pii: true
    register_error_listener: false
    register_error_handler: false

monolog:
    handlers:
        sentry:
            type: sentry
            level: !php/const Monolog\Logger::ERROR
            hub_id: Sentry\State\HubInterface

All children of HttpExceptionInterface (NotFoundHttpException, BadRequestHttpException and so on...) are sent to sentry and notify the team : false positives.

@cleptric
Copy link
Member

I recommend reverting back to IgnoreErrorsIntegration if you rely on the is_a check. In the next major version, we'll add this to ignore_exceptions as well as RegEx support.

@cleptric cleptric reopened this Oct 17, 2023
@cleptric cleptric changed the base branch from master to 4.x October 17, 2023 23:39
@cleptric cleptric self-assigned this Oct 17, 2023
@cleptric cleptric added this to the 4.0 milestone Oct 18, 2023
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.

Excellent change. Going to make it much easier to ignore a certain class of exceptions 👍

@stayallive stayallive changed the title Rollback to is_a() to ignore exceptions instead of in_array() [4.x] Rollback to is_a() to ignore exceptions instead of in_array() Oct 18, 2023
@cleptric
Copy link
Member

Will fix CI later.

@cleptric cleptric merged commit 9240c30 into getsentry:4.x Oct 18, 2023
32 of 33 checks passed
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.

3 participants