phptek / silverstripe-sentry

Flexible Sentry compatible bug aggregation client for Silverstripe applications.
BSD 3-Clause "New" or "Revised" License
11 stars 22 forks source link

Performance Monitoring #93

Open samandeggs opened 5 months ago

samandeggs commented 5 months ago

Hey there - is there any intention, or ability to out of the box include performance monitoring? I couldn't seem to trigger it using standard implementation, and playing around with the repo I couldn't quite figure out where the necessary pieces would align to trigger this.

Laraval's docs are here: https://docs.sentry.io/platforms/php/guides/laravel/performance/?original_referrer=https%3A%2F%2Fdocs.sentry.io%2Fproduct%2Fperformance%2Fgetting-started%2F

But they include a package themselves (sentry) for its support.

If someone can point me in the right direction I'll be happy to look into making a PR, however I am working within the context of SS4 still so may not be exactly what everyone's after.

phptek commented 5 months ago

Interesting idea. Off the top of my head, you'd prob start looking at SentryAdaptor::setContext() as that's where Silverstripe-land, hands off to the Sentry SDK. So you might look to add another case statement for e.g. traces_sample_rate and traces_sampler but I haven't spent more than ~10m thinking about it just now :-)

Also bear in mind any caveats with accuracy, given that as Sentry themselves say, data is returned to the app before it's sent to Sentry so there's going to be some apparent latency AFAICT.

Other than that, if you can develop a patch, I'll happily review it :-)

UPDATE:

Poke around in SentryLogger::factory() as that's how setContext() (above) gets called. You would prob create a new method defaultTrace() or something and pass that into factory() ala:

...
        $trace = $this->defaultTrace();
        ...
        $adaptor = (new SentryAdaptor($client))
            ->setContext('env', $env)
            ->setContext('tags', $tags)
            ->setContext('extra', $extra)
            ->setContext('user', $user)
            ->setContext('traces_sampler', $trace);
samandeggs commented 5 months ago

Thanks @phptek - I've managed to get it firing, I just need to test that the sampling rate is actually being respected - as well as some refinement in how it's being presented in Sentry itself, then I'll try to put together a PR for you to review if you're interested. I am working on the SS4 version (this package's 4.1.1 tag), though - which is the context of the project I needed to figure this out for.

However, it doesn't look like there's too much of a difference between versions but if you're not keen on adding additional version on the 4 tags, let me know now so I can just pass you the examples here if you want to bring it into the 5 version for yourself.

phptek commented 5 months ago

Thanks and yes I am interested. Just issue your eventual PR against master. FYI there's no real difference between ^4 and ^5 it's kind of arbitrary TBH, those tags just mostly demarcate changes in the underliying sentry/sentry SDK.

phptek commented 2 months ago

@samandeggs Any progress on this work? Happy to review PRs :-)

samandeggs commented 2 months ago

Hi @phptek I did do quite a bit of work on this, but I have ended up not being able to find the time to complete things. The final piece of work that needed completion was maintaining the lifecycle of the transaction properly - presumably with an injection into the request handler itself.

The naming, timing and data was all coming through, but the actual time of the transaction was being opened and closed immediately, and I need to open it at the start of the request, and close at the end of it - which was likely not a very complex task to complete but I simply haven't gotten around to it.

If you're curious or want to help finishing it up I can pass through a PR. It wont be exactly done, however.