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

Errors not getting grouped when they are the same from inside blade files #513

Open
olivernybroe opened this issue Sep 28, 2021 · 9 comments

Comments

@olivernybroe
Copy link

Environment

How do you use Sentry?
Sentry SaaS (sentry.io)

Which SDK and version?
sentry.php.laravel 2.8.0

Steps to Reproduce

Whenever an error happens from inside a blade file, they are added as a new issue, if the error was in a different version. (we clear blade cache on each release)
image

When looking at the stacktrace, the reason it doesn't group them might be because of param0, which contains the hashed id of the cached blade file. This means when we clear the cache, that id will be different, and sentry will detect it as a different issue.
image

Another plausible reason could be that sentry reports that the stacktrace cannot be found for issues, as long as the issue happens inside a blade file. (warning is not shown in non blade files)

image

@olivernybroe
Copy link
Author

Looking at this a bit closer, I think this might be the fault of ignition, as ignition wraps all the blade exceptions into a Facade\Ignition\Exceptions\ViewException.

So this might be solvable by removing ignition from production 🤔

@stayallive
Copy link
Collaborator

stayallive commented Sep 28, 2021

Removing Ignition from production seems like a great plan anyway unless you have a specific reason for having it there to prevent having multiple error handles, moving it to require-dev should be enough if you are just using it for the pretty debug screens.

The trace not being found should only happen if you are using performance tracing and the transaction is not or not correctly set or the trace was never sent (are you using sampling) and that might be a separate issue all together.

But removing Ignition will not solve it, we need to strip or normalize the view paths from the exception message for grouping to function correctly which we are not doing at the moment. The frame paths are correct but I actually suspect that is because of Ignition :)

I'll leave this open as a feature request, we need to both cleanup the exception message and the stack frame paths to show the actual blade file paths instead of the compiled paths.


I do have a "solution" in the meantime that fixes the exception messages but it ain't pretty and I did not had the time to package this up nicely for the SDK yet:

<?php

namespace App\Exceptions;

use Sentry\Event;
use Sentry\ExceptionDataBag;

class Sentry
{
    public static function beforeSend(Event $event): ?Event
    {
        // The view paths are retrieved from the config and we add the base path to replace after
        $pathsToRemove = array_map(function ($path) {
            return rtrim($path, '/') . '/';
        }, array_merge(config('view.paths'), [realpath(base_path())]));

        // Normalize messages with the (view) paths in them to help with grouping
        $event->setExceptions(array_map(static function (ExceptionDataBag $exception) use ($pathsToRemove) {
            if (!empty($exception->getValue())) {
                $exception->setValue(str_replace($pathsToRemove, '', $exception->getValue()));

                preg_match('/[a-zA-Z\-_\/]+\.blade\.php/', $exception->getValue(), $matches);

                if (!empty($matches[0])) {
                    $viewName = str_replace(['/', '.blade.php'], ['.', ''], $matches[0]);

                    $exception->setValue(str_replace($matches[0], $viewName, $exception->getValue()));

                    $occurrences = substr_count($exception->getValue(), $searchViewName = "(View: {$viewName})");

                    if ($occurrences > 0) {
                        $exception->setValue(trim(self::str_replace_n_occurences($searchViewName, '', $exception->getValue(), $occurrences - 1)));
                    }
                }
            }

            return $exception;
        }, $event->getExceptions()));

        return $event;
    }

	private static function str_replace_n_occurences(string $from, string $to, string $content, int $n = 1): string
	{
	    $from = '/' . preg_quote($from, '/') . '/';

    	return preg_replace($from, $to, $content, $n);
	}
}

And add this to your sentry.php config file:

    'before_send' => [App\Exceptions\Sentry::class, 'beforeSend'],

This will convert view exceptions with the view path to something like this:

foreach() argument must be of type array|object, null given (View: media.partial.proxy.layout.default)

We already have code that does this much cleaner but that is only used currently to have a nice backtrace path for SQL query origins in performance tracing.

@olivernybroe
Copy link
Author

@stayallive Yeah, I think it might make sense to either support running with ignition or add to the docs that ignition should a dev-dependency. At least because ignitions own docs recommends installing as a dependency not a dev dependency.

I'll move it to a dev-dependency so that part is at least out of the way 👍

Sounds nice with a "solution". Hahah, yeah that's not pretty ;)
Consider taking a look at how ignition solved this, as they also show the original blade file.
https://github.com/facade/ignition/blob/main/src/Views/Engines/CompilerEngine.php
https://github.com/facade/ignition/blob/main/src/Views/Concerns/CollectsViewExceptions.php

Could be nice if we could also extend to showing the variables given to the view file like ignition does 👍

@stayallive
Copy link
Collaborator

Keep in mind that the reason Ignition is recommended as a normal dependency because it's the client for Flare too.

I'm also not seeing how it's not supported to run with Ignition installed, everything works right?

We need to do some changes regardless if Ignition is installed or not.

@olivernybroe
Copy link
Author

True. We just need to handle that the ViewException could also be a ignition ViewException.

@stayallive
Copy link
Collaborator

Oh right, yeah for sure 👍

@kamilogorek
Copy link
Contributor

You can also use fingerprints to group events together. Either on the SDK side via https://docs.sentry.io/platforms/php/usage/sdk-fingerprinting/ or using built-in fingerprint rules, which can definitely fix the problem of variable ID being used here - https://docs.sentry.io/product/data-management-settings/event-grouping/fingerprint-rules/

@stayallive
Copy link
Collaborator

@kamilogorek yes good point, that is also a possibility to fix the grouping but not having to alter the exception message.

@github-actions

This comment was marked as outdated.

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

No branches or pull requests

4 participants