Introduce Sentry\Laravel\trace custom span helper#598
Introduce Sentry\Laravel\trace custom span helper#598stayallive wants to merge 5 commits intomasterfrom
Sentry\Laravel\trace custom span helper#598Conversation
e726b8b to
3a8772e
Compare
3a8772e to
8ba785c
Compare
|
Name bikeshedding, I'd not call |
|
Yeah good point, this was totally a placeholder name but something like Maybe it could be as simple as (also the performance metrics feature was unknown to me until now 👀) |
Sentry\Laravel\measure custom span helperSentry\Laravel\trace custom span helper
src/Sentry/Laravel/functions.php
Outdated
| // active currently. If that is the case we don't create a unused | ||
| // span and we immediately execute the callable and return the result | ||
| if ($parentSpan === null) { | ||
| return $trace(null); |
There was a problem hiding this comment.
can we move this logic into the withScope call? This way we don't need to make the getCurrentHub call at the top.
There was a problem hiding this comment.
This pushed/pops potentially a useless scope, but it's probably cheap enough to not care too much about so I've changed it 👍 (also removed the nullable scope, so that is great)
| } | ||
|
|
||
| return $trace($scope); | ||
| }); |
There was a problem hiding this comment.
don't we need to finish the span after the callback is called?
also - do we need tests?
There was a problem hiding this comment.
🤦♂️ yes, the scope just pops not finishes, andh yes we need tests, this is just a PoC.
We wanted to wait to see if this would be generally a good idea at all before actually pushing it so it's the bare minimum, ideally this would even go in the base PHP SDK.
There was a problem hiding this comment.
@stayallive I think @cleptric and I are going to try writing an RFC to add this as unified API, would you like to help contribute?
|
Proposal moved to getsentry/sentry-php#1490 |
Adding a helper method that reduces a lot of boilerplate of adding spans for user-code to a application, allowing users to quickly wrap existing calls with a span and it looks something like this:
The
SpanContextis a unfortunate object in our API at the moment and doesn't have a fluent interface so I've used thetap()helper from Laravel to keep it a little more readable.In this exampel we also pass the current scope to the callback that is being traced, this might be a good idea or not... maybe the user should wrap this block in their own
withScopebut it seems easy enough to ignore if you don't want to do anything with the scope.(this is inspired by a homegrown method I'm using in my applications to make this easier but simplified to make it more general and acceptable)
This is very much a concept and naming might probably change a lot in the process.