diginov / craft-sentry-logger

Pushes Craft CMS logs to Sentry through a real Yii 2 log target.
MIT License
5 stars 7 forks source link

Doesn't catch bootstrap errors #13

Open timkelty opened 5 months ago

timkelty commented 5 months ago

First – I've been cobbling together my own Sentry integrations for Craft for far too long, and am very grateful I finally found this plugin! Thank you!

And, apologies for posting as a bug – this is more of a FR. Might be nice to enable discussions on this repo? 👼

Description

I like to init my Sentry client as early as possible (as recommended) so bootstrap errors can still be caught.

This log target creates a new client on init, which means I can still do this, but I essentially have to configure the client twice, once in my app bootstrap with \Sentry\init, and once within config/app.php.

Essentially, a shortcut to doing this:

<?php

use craft\helpers\App;
use diginov\sentrylogger\log\SentryTarget;

return [

    'components' => [
        'log' => [
            'targets' => [
                'sentry' => function() {
                    if (!class_exists(SentryTarget::class)) {
                        return null;
                    }

                   // Sentry client is pre-configured in `boostrap.php` via \Sentry\init
                   $clientOptions = Sentry\SentrySdk::getCurrentHub()->getClient()->getOptions();

                    return Craft::createObject([
                        'class' => SentryTarget::class,
                        'enabled' => App::env('CRAFT_ENVIRONMENT') !== 'dev',
                        'anonymous' => false,
                        'dsn' => $clientOptions->getDsn(),
                        'release' => $clientOptions->getRelease(),
                        'environment' => $clientOptions->getEnvironment(),
                        'options' => $clientOptions,
                        'levels' => ['error', 'warning'],
                        'exceptCodes' => [403, 404],
                        'exceptPatterns' => [],
                    ]);
                },
            ],
        ],
    ],

];
martinleveille commented 5 months ago

Hi @timkelty,

Thank you for the good words, it’s really appreciated!

If I understand correctly, you want to be able to use the plugin by only having \Sentry\init(['dsn' => '___PUBLIC_DSN___' ]); in your bootstrap.php file.

Basically, the idea would be to add logic in the Plugin.php file to detect if the SDK already has a client initialized with \Sentry\SentrySdk::getCurrentHub()->getClient().

The only negative point of adding this logic in Plugin.php is that the initialization is done later than the advanced configuration of the log component in the app.php file.

If it's okay with you, I can add this logic into the Plugin.php file without much trouble.

timkelty commented 5 months ago

you want to be able to use the plugin by only having \Sentry\init(['dsn' => '_PUBLICDSN' ]); in your bootstrap.php file

I suppose it could, but I was expecting that I would still need to register the log target (advance configuration), but I wasn't thinking about the other methods the plugin allows.

For my uses, here's what I would ideally want to work:

// bootstrap.php
\Sentry\init([
  'dsn' => '___PUBLIC_DSN___' ,
  'release' => App::env('GIT_SHA'),
  'sample_rate' => 0.1,
]);
<?php
// config/app.php
use craft\helpers\App;
use diginov\sentrylogger\log\SentryTarget;

return [
    'components' => [
        'log' => [
            'targets' => [
                'sentry' => function() {
                    if (!class_exists(SentryTarget::class)) {
                        return null;
                    }

                   // Sentry client is pre-configured in `boostrap.php` via \Sentry\init
                   $clientOptions = Sentry\SentrySdk::getCurrentHub()->getClient()->getOptions();

                    return Craft::createObject([
                        'class' => SentryTarget::class,
                        'enabled' => App::env('CRAFT_ENVIRONMENT') !== 'dev',
                        'anonymous' => false,
                        'levels' => ['error', 'warning'],
                        'exceptCodes' => [403, 404],
                        'exceptPatterns' => [],
                    ]);
                },
            ],
        ],
    ],

];

However, maybe it would make more sense for the plugin to provide its own static method, eg, diginov\sentrylogger\log\SentryTarget::bootstrap, so the interface could be the same and you could have all your config in one place (in your bootstrap).

leevigraham commented 2 weeks ago

@timkelty @martinleveille I think adding it to the bootstrap would also improve transactions / profiling.

martinleveille commented 2 weeks ago

Hi @timkelty and @leevigraham,

Thank you for your feature request! I understand the value of your suggestion and I appreciate your input. Unfortunately, I don’t have the time to implement this myself at the moment. However, I’d be happy to review a pull request if you’d like to contribute. Feel free to submit one if you’re interested!

Thanks again for your support!