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

Log error aren't going to Bugsnag #301

Open
dbpolito opened this issue May 3, 2018 · 7 comments
Open

Log error aren't going to Bugsnag #301

dbpolito opened this issue May 3, 2018 · 7 comments
Labels
needs discussion Requires internal analysis/discussion

Comments

@dbpolito
Copy link

dbpolito commented May 3, 2018

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

  • laravel/framework: 5.4.36
  • bugsnag/bugsnag-laravel: 2.14.1

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

dbpolito commented May 3, 2018

@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
Copy link
Author

dbpolito commented May 3, 2018

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
Copy link
Author

dbpolito commented May 7, 2018

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 #301 (comment), 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
Copy link
Contributor

Cawllec commented May 10, 2018

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
Copy link

matthew-inamdar commented Nov 20, 2018

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.

@abigailbramble abigailbramble added the needs discussion Requires internal analysis/discussion label Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires internal analysis/discussion
Projects
None yet
Development

No branches or pull requests

5 participants