Do not load Sentry at all if the DSN is empty or not set

[stayallive]

Currently it looks like Sentry is still loaded (although it's not sending errors) when the DSN is not set.

This can cause errors originating from other parts of the application to show with a stack trace coming from Sentry, we should prevent Sentry from loading at all if we don't have a DSN since we are not helpful in that case.

Ref #103

[garygreen]

Really, Sentry shouldn't be loaded at all prior to sending any errors - why does it need to load? I think there was a discussion about this in a previous issue somewhere (can't seem to find it) but the SentryLaravelEventHandler should really just be a repository for collecting information about the application (breadcrumbs etc) and then when an exception needs to be reported, THEN load sentry with the data. Seems wasteful to load Sentry on every single request when an exception should be seen as... well... an exceptional case, IMO

The sentry raven client isn't exactly lightweight so this would be a nice improvement.

Edit - Found the discussion, it's in: https://github.com/getsentry/sentry-laravel/issues/73#issuecomment-311314324

[garygreen]

One of the concerns of defering the loading of the Sentry client was that it may not send data if Sentry was used directly, as the data wouldn't of been attached.

A way around that could be to add some kind of 'beforeSend' event to the sentry client itself, then register a callback on the sentry client to fire the callbacks before sentry is about to send the data.. then you can attach all the needed data.

Only issue I can think with that is if when logging the breadcrumbs do they log in the correct order with the right timestamps? I'm not sure of the exact implementation details but I'd imagine it's possible to replay / give a specific order to the breadcrumbs and specific timestamps to solve that.

Sentry::beforeSend(function($client) {
   SentryLaravelEventHandler::attachData($client);
});

[stayallive]

It would be very redundant if we implement our own breadcrumb logic and only if something happens start up the Sentry client and copy the data over and send it to Sentry, there are a few reasons for this I can think of (really fast).

• We would be implementing lot's of the code twice. Once in the Client since non-laravel users still need it ofcourse and once in the this package making for a lot of duplication
• The Client currently hooks into the shutdown handler which Laravel does not catching out of memory exceptions and the like (if we would, again double the code)

With all the duplicated code I don't believe it will be much of an gain.

Happy to run (or see) some benchmarks to see how much the Sentry client really bogs Laravel down before we start optimizing problems that might not be there 😉

[garygreen]

Those are fair points. Just done a test to see if there is really a performance / memory problem and this is what I've found:

Sentry Page 1: 10.98MB -> 11.07MB - 10 queries +0.81% Page 2: 11.87MB -> 12.12MB - 20 queries +2.10% Page 3: 13.70MB -> 14.09MB - 100 queries +2.84%

Compare this to Bugsnag (which I'm currently using)

Bugsnag Page 1: 10.98MB -> 10.98MB - 10 queries +0% Page 2: 11.87MB -> 11.89MB - 20 queries +0.16% Page 3: 13.70MB -> 13.81MB - 100 queries +0.80%

For memory, there is a larger memory footprint for pages using Sentry particularly when you have a lot of queries.

In terms of time, there isn't much difference between either but there is a bit more of a "spike" with Sentry as compared to the control times and Bugsnag.

These numbers aren't that large though so might not be worth worrying about.

[stayallive]

I will definitely take this in account, there is also a lot of work going on for the 2.0 version of the PHP SDK that will be profiled if done to see if that improves things and if not see if we can improve on that 👍

Thanks for running those number btw!

[mfn]

I ~used~ tested the https://github.com/NoiseByNorthwest/php-spx profiler and was surprised to find this output when running the unit tests (in the top ten):

…
1.11s | 607.5ms | -37.8MB | 30.1MB | 68.2K | Sentry\\SentryLaravel\\SentryLaravelEventHandler::queryExecutedHandler
…

There's no DSN set for tests so I was surprised the code was invoked at all.

I'm not sure I understand from the current discussions what the goals are here, but wouldn't simple check in \Sentry\SentryLaravel\SentryLaravelServiceProvider::register for whether thesentry.dsn is set or not suffice to early return the register method? As in:

    public function register()
    {
        $this->app->singleton(static::$abstract . '.config', function ($app) {
            // Make sure we don't crash when we did not publish the config file and the config is null
            return $app['config'][static::$abstract] ?: array();
        });

        $dsn = config('sentry.dsn');
        if (!$dsn) {
            return;
        }

🤷‍♀️

[stayallive]

This took a while to get back to but I think not registering the events is the way to go forward.

If you want that use a DSN if you are doing custom voodoo Sentry can still be called but does nothing out of itself (it's not registering anything except the singeton).

Might this (#104) be a good compromise?

[garygreen]

@stayallive I notice that the code behind Sentry has improved/been refactored a lot recently and that the main Client seems to implement what was spoken about in this suggestion - it appears to be only collecting the data it needs to send off to Sentry and it will only initialise it once it actually sends the report/exception? If that's the case, is this issue now resolved or am I mistaken?

Out of interest, it would be great if you could summarise what's changed about Sentry internally to improve on deferring loading classes, etc. I notice that your Javascript SDK is doing a similar technique by only being a "data collector" and then initialising Sentry when there is an actual exception to report: https://docs.sentry.io/platforms/javascript/?platform=browser#lazy-loading-sentry

This is really great to see 👍

[stayallive]

Hey @garygreen, yes v2 of the PHP SDK was a big overhaul indeed.

About what that means for the Laravel integration:

It still initializes the Sentry client (and with it - unhandled/fatal error tracking) in the beginning of the request and collects breadcrumbs (but only when a DSN value was found) from Laravel events as they happen.

So yes it was refactored a bunch an brings a lot of nice additions and the possibility to enable async sending of the events more reliable than it was but it still collects it's data waiting for a possible unhandled exception.

However the breadcrumb structure in Sentry is just an array with data, so it does not do much processing just a bit of memory to keep track of the data, however I do not have exact numbers again but it's not like the lazy loading in JavaScript (which also has some downsides).

As mentioned before the reason is that the PHP SDK is not just a "curl wrapper" but also handles all the heavy lifting of registering for unhandled and fatal errors that occur, if we would not load Sentry at all until something happens we are to late or need te re-implement all that handler stuff in the Laravel SDK which makes no sense, so I'm not still 100% convinced we can do better than what we are doing right now, but open to ideas ofcourse.

However, the issue was to not load Sentry at all if there is no DSN set was the main issue here and that is partially solved by not loading any Sentry specific integrations in Laravel if the DSN was not set, this solved this issue IMO, possibly if there are ideas on how to improve the performance or async-ness of the SDK should be continues at the PHP SDK or possibly in a new issue here if there are specific ideas (like adding a configuration option to "disable" breadcrumbs and thus not doing anything until a error occurs like JavaScript lazy loading works)?

[railsfactory-robin]

Sentry.init({ dsn: 'http://public key@localhost:9000/projectid', maxBreadcrumbs: 50, debug: true, }) // project id can be find in settings-> projects-> client keys->configure keys

[stayallive]

Closing this issue because it is quite old and it has been a long time since there was any activity, if there is still a problem please open a new issue.