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

Caught exceptions are send to sentry #113

Open
amenk opened this issue Mar 6, 2023 · 8 comments
Open

Caught exceptions are send to sentry #113

amenk opened this issue Mar 6, 2023 · 8 comments
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@amenk
Copy link
Contributor

amenk commented Mar 6, 2023

Bug: What is the current behavior?

When opening the page with clear layout cache, lots of exceptions are sent to sentry, which are actually caught be Magento core already.

Bug: What is the expected behavior?

Not sending these exceptions to Magento.

Bug: What is the proposed solution?

Filter them out?

image

Additional

In general I am also wondering why these exceptions are raised at all, but this is a different story for StackOverflow.

  • Magento 2.4.5-p1
  • Module 3.3.0
@indykoning
Copy link
Member

These are errors that are not actually caught by Magento until the very last moment, at a point where it's not possible to hook into it anymore.
Then depending on the context it hides this error.
If it were in the terminal it would still error out and break the application.

There's a large chance to miss important errors if we were to ignore the errors that would be hidden in the frontend or otherwise solved (by for instance adding a notification)

Normally you can filter exceptions with these settings: https://github.com/justbetter/magento2-sentry#:~:text=%27log_level%27%20%3D%3E%20%5CMonolog%5CLogger%3A%3AWARNING%2C%0A%20%20%20%20%27errorexception_reporting%27%20%3D%3E%20E_ALL%2C%0A%20%20%20%20%27ignore_exceptions%27%20%3D%3E%20%5B%5D%2C
But since it's a very general ErrorException that's not smart.

I am thinking about adding wrappers for the filtering functions https://docs.sentry.io/platforms/php/configuration/filtering/
so Magentos plugin system can be used to change and filter them.

But right now you can already add advanced filtering in the following way
#89 (comment)

@amenk
Copy link
Contributor Author

amenk commented Mar 7, 2023

These are errors that are not actually caught by Magento until the very last moment, at a point where it's not possible to hook
into it anymore.

I am confused 😕 ? There is a catch in line 996 and no throw - so I believe it should be caught?

@amenk
Copy link
Contributor Author

amenk commented Mar 14, 2023

@indykoning I think that exception is caught - do you know the reasons why it still pops up? Is it because it's originally a warning?

@indykoning
Copy link
Member

The catch statement does not catch every throwable sadly so things could still pass through, throwable is the base of any exception which is what we catch so that could be a cause.
Or some Magento magic is obscuring the true stacktrace, they have an error handler that transforms any runtime error or warning into an exception which could be a probable cause of it passing through the first catch as it's a warning but not through Sentry as it's an exception by that point.

Sadly i'm not able to reproduce these errors in my projects

@amenk
Copy link
Contributor Author

amenk commented Mar 14, 2023

Happy to help debugging.

It looks like the warning is converted to an exception in

vendor/magento/framework/App/ErrorHandler.php

which seems to be logged by sentry before it's converted to an exception and being caught.

@amenk
Copy link
Contributor Author

amenk commented Mar 14, 2023

I would have a workaround for that specific problem - adding the NOWARNING and NOERROR flags in https://github.com/magento/magento2/blob/b9101b158a9acb163a31433b2c20fe13cad5f7fe/lib/internal/Magento/Framework/View/Model/Layout/Merge.php#L564

        return simplexml_load_string($xmlString, \Magento\Framework\View\Layout\Element::class,  LIBXML_NOWARNING | LIBXML_NOERROR);

I could submit a patch to Magento, but it is possible that this happens also on other places.

The question is now to not report such errors which are caught by Magento Error handler.

Probably it is reproducible by changing this line to

        return simplexml_load_string('brokenXMLStringHere', \Magento\Framework\View\Layout\Element::class);

@amenk
Copy link
Contributor Author

amenk commented Mar 14, 2023

It's sent to sentry here:

vendor/sentry/sentry/src/ErrorHandler.php:302

I am reproducing this by inserting this code:

vendor/magento/framework/App/Bootstrap.php:267

                $response = $application->launch();

                # test code here, after launch:
                try {
                    simplexml_load_string('thisisanerror', \Magento\Framework\View\Layout\Element::class);
                } catch (\Exception $e) {
                    echo $e->getMessage();
                }

                die(__METHOD__);

Error handler is registered here:

# vendor/sentry/sentry/src/ErrorHandler.php:143
        $errorHandlerCallback = \Closure::fromCallable([self::$handlerInstance, 'handleError']);

        self::$handlerInstance->isErrorHandlerRegistered = true;
        self::$handlerInstance->previousErrorHandler = set_error_handler($errorHandlerCallback);

So this is sentry core code. I would be better from my point of view if the sentry error handler runs after custom framework (Magento) error handlers to see what was caught already and to avoid false positives?

@amenk
Copy link
Contributor Author

amenk commented May 19, 2023

I did some experiments with registering the Magento handler after Sentry's, by patching:

\Magento\Framework\App\Bootstrap::run

    public function run(AppInterface $application)
    {
        try {
            try {
                \Magento\Framework\Profiler::start('magento');
                $this->assertMaintenance();
                $this->assertInstalled();



                $response = $application->launch();
                $this->initErrorHandler();

                foobar();
                
                try {
                    simplexml_load_string('thisisanerror', \Magento\Framework\View\Layout\Element::class);
                } catch (\Exception $e) {
                    echo $e->getMessage();
                }

While this reporting the caught exception of SimpleXML ist would also avoid reporting the non existing foobar() method because this is also actually a caught exception and Magento uses exit(1) in \Magento\Framework\App\Bootstrap::terminate

@indykoning indykoning added bug Something isn't working wontfix This will not be worked on labels Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants