brefphp / laravel-bridge

Package to use Laravel on AWS Lambda with Bref
https://bref.sh/docs/frameworks/laravel.html
MIT License
314 stars 63 forks source link

Stack channel logging issue #27

Open gusiq opened 3 years ago

gusiq commented 3 years ago

Hey! I'm having an issue when using the package because of line 46 of BrefServiceProvider:

// We change Laravel's default log destination to stderr
$logDriver = Config::get('logging.default');
if ($logDriver === 'stack') {
    Config::set('logging.default', 'stderr');
}

We're using multi-channel logging, and our stack channel already include stderr. Config bellow:

'stack' => [
    'driver' => 'stack',
    'channels' => ['stderr', 'bugsnag'],
    'ignore_exceptions' => false,
],

When using the package the bugsnag just stop working.

Couldn't we just leave this to user or at least change approach to just ensure stack contains stderr instead of changing logging.default (like below example)?

// We change Laravel's default log destination to stderr
$logDriver = Config::get('logging.default');
if ($logDriver === 'stack') {
    $channels = Config::get('logging.channels.stack.channels', []);
    array_push($channels, 'stderr');
    Config::set('logging.channels.stack.channels', array_unique($channels));
}
mnapoli commented 3 years ago

Hi, that's an interesting approach and I like it.

However, we need to make sure that the logger does not try to write to disk: would you solution make sure of that?

gusiq commented 3 years ago

Well we could filter channels to remove all that use filesystem as storage. User still could use some custom channel that writes to disk, but I think the risk is very low.

// We remove single and daily channels and ensure that stderr is used
$logDriver = Config::get('logging.default');
if ($logDriver === 'stack') {
    $channels = array_filter(Config::get('logging.channels.stack.channels', []), function ($value) {
        return !in_array($value, ['single', 'daily']);
    });
    array_push($channels, 'stderr');
    Config::set('logging.channels.stack.channels', array_unique($channels));
}

It's not a perfect solution, but I think can solve most use cases. Syslog and errorlog aren't a problem, right? What do you think?

mnapoli commented 3 years ago

Right, that makes sense. However that sounds fragile to me: new versions of Laravel could very well rename single to something else for example.

I would love to find a solution that's a bit more reliable for the future, but I'm not sure what/how 🤔

bkuhl commented 3 years ago

As one who also uses Bugsnag and is looking to use this project in production, I'd rather accept the risk of the filesystem being implemented in a Laravel upgrade where my radar is up for potential issues than sacrifice Bugsnag support. So I'd be a fan of just eliminating the logging file aliases.

JonathanGuo commented 3 years ago

The variable name $logDriver is a bit misleading. The code reads from the configuration 'logging.default', which is actually the channel name.

I think the easiest solution just set the LOG_CHANNEL environment variable to be a custom one, e.g. lambda-stack, and put only stderr and bugsnag into the channels array.

        'lambda-stack' => [
            'driver' => 'stack',
            'channels' => [
                'bugsnag',
                'stderr',
            ],
            'ignore_exceptions' => false,
        ],
Zakini commented 2 years ago

Just got stung by this issue while trying to add Flare to the stack log channel.

One solution might be to add a new log channel named serverless to the logging config, then change the code snippet in the OP to always set the default logging channel to serverless. By default that could only include the stderr driver, but users could add whatever other drivers they want as well. The docs would need to point this out and mention that channels that expect access to a filesystem shouldn't be used.

In any case, this issue definitely needs mentioning in the docs.