bugsnag / bugsnag-laravel

BugSnag notifier for the Laravel PHP framework. Monitor and report Laravel errors.
https://docs.bugsnag.com/platforms/php/laravel/
MIT License
874 stars 129 forks source link

Log error aren't going to Bugsnag #301

Open dbpolito opened 6 years ago

dbpolito commented 6 years ago

Expected behavior

logger()->error('test');

Should go to Bugsnag

Observed behavior

It's not going to Bugsnag

Steps to reproduce

php artisan tinker
logger()->error('test');

Version

Additional information

My AppServiceProvider:

<?php

namespace App\Providers;

use Bugsnag\BugsnagLaravel\BugsnagServiceProvider;
use Bugsnag\BugsnagLaravel\Commands\DeployCommand;
use Bugsnag\BugsnagLaravel\Facades\Bugsnag;
use Illuminate\Contracts\Logging\Log;
use Illuminate\Support\ServiceProvider;
use Psr\Log\LoggerInterface;

class AppServiceProvider extends ServiceProvider
{
    public function register()
    {
        if ($this->app->environment('local')) {
            return;
        }

        $this->registerBugsnag();
    }

    protected function registerBugsnag()
    {
        $this->app->register(BugsnagServiceProvider::class);
        $this->commands(DeployCommand::class);

        $this->app->alias('bugsnag.multi', Log::class);
        $this->app->alias('bugsnag.multi', LoggerInterface::class);
        $this->app->alias('Bugsnag', Bugsnag::class);
    }
}
GrahamCampbell commented 6 years ago

Thanks for getting in touch. Dynamically registering the bugsnag service provider won't work. You should instead use release stages to exclude local from notifying.

Also, you should remove $this->app->alias('Bugsnag', Bugsnag::class); as it won't have the correct affect of doing a facade alias, which is what I assume you wanted. You'll need to use the aliases in the config/app.php file.

GrahamCampbell commented 6 years ago

By default Bugsnag won't actually attempt to notify if the env is local actually. Please see the docs at https://docs.bugsnag.com/platforms/php/laravel/configuration-options/#release-stage.

dbpolito commented 6 years ago

@GrahamCampbell This was working until now, what changed to not work anymore?

I only noticed this isn't working today, i can't really confirm since when it stopped to work.

But i will update my code.

dbpolito commented 6 years ago

Also, looking at the code there is no default release stage anymore, on Bugsnag or Bugnag-laravel: https://github.com/bugsnag/bugsnag-php/blob/b1cd36a49a5a780824d9b3f369ad89275013b19f/CHANGELOG.md#107

So setting BUGSNAG_NOTIFY_RELEASE_STAGES is required to ignore local, right?

dbpolito commented 6 years ago

So my findings here are:

logger() is not working from 5.3 to 5.5

Because of this:

https://github.com/laravel/framework/blob/5.4/src/Illuminate/Support/Facades/Log.php#L17-L20 https://github.com/laravel/framework/blob/5.4/src/Illuminate/Foundation/helpers.php#L495-L502

The helper calls app('log') which isn't aliased to Bugsnag one, so it doesn't work at all, my workaround was at file bootstrap/app.php:

<?php

define('LARAVEL_START', microtime(true));

/*
|--------------------------------------------------------------------------
| Register The Composer Auto Loader
|--------------------------------------------------------------------------
|
| Composer provides a convenient, automatically generated class loader
| for our application. We just need to utilize it! We'll require it
| into the script here so that we do not have to worry about the
| loading of any our classes "manually". Feels great to relax.
|
*/

// @TODO: temporary fix for:
// https://github.com/bugsnag/bugsnag-laravel/issues/301

/**
 * Log a debug message to the logs.
 *
 * @param  string  $message
 * @param  array  $context
 * @return \Illuminate\Contracts\Logging\Log|null
 */
function logger($message = null, array $context = [])
{
    $logClass = \Illuminate\Contracts\Logging\Log::class;

    if (is_null($message)) {
        return app($logClass);
    }

    return app($logClass)->debug($message, $context);
}

require __DIR__.'/../vendor/autoload.php';

We could open a PR to Laravel to fix, but they are old version, not sure if it will be merged, i'm using 5.4 here.

Re: By default Bugsnag won't actually attempt to notify if the env is local actually.

This is incorrect as i imagined when i posted https://github.com/bugsnag/bugsnag-laravel/issues/301#issuecomment-386401599, local env are being sent to Bugsnag, even with APP_DEBUG=true.

If i add this to my env:

BUGSNAG_NOTIFY_RELEASE_STAGES=development,production

Then local stop, maybe good to update docs to make things more clear.

Tinker is never tracked

Which i think it's good and expected.

My Provider was working properly

I reverted to my old provider and nothing changed, so my fix worked on my setup but you are correct about the alias, which wasn't working.


This logger() thing is a big thing for me, as we use it a lot across the system as we weren't being notified about it, something to look into.

Cawllec commented 6 years ago

Hi @dbpolito, I've confirmed your findings, and I'm glad you've found a workaround. However this may be an issue we end up documenting, unless we can think of a non-intrusive solution. I'll discuss it with my colleagues.

matthew-inamdar commented 5 years ago

If anyone else is facing this issue on Laravel 5.6, another thing to note is that the errors don't seem to send when using Tinker.

Setup a temp route which logs the error and then access it from the web. This solved it for me.