Skip to content

Introduce Sentry\Laravel\trace custom span helper#598

Closed
stayallive wants to merge 5 commits intomasterfrom
propose-span-helper
Closed

Introduce Sentry\Laravel\trace custom span helper#598
stayallive wants to merge 5 commits intomasterfrom
propose-span-helper

Conversation

@stayallive
Copy link
Collaborator

@stayallive stayallive commented Oct 17, 2022

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:

use Sentry\State\Scope;
use Sentry\Tracing\SpanContext;
use function Sentry\Laravel\trace;

$args = [...];

$resutl = trace(
    function (Scope $scope) use ($args) {
        $scope->setTag('some-tag', 'some-value');

        return $this->doSomethingMeasureable($args);
    },
    tap(new SpanContext, function (SpanContext $context) use ($args) {
        $context->setOp('something.measurable');
        $context->setData(['context' => $args]);
        $context->setDescription('Something measurable');
    }),
);

The SpanContext is a unfortunate object in our API at the moment and doesn't have a fluent interface so I've used the tap() 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 withScope but 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.

@marandaneto
Copy link

Name bikeshedding, I'd not call measure because it could be mistaken as measurements and performance metrics.
https://docs.sentry.io/platforms/dart/performance/instrumentation/performance-metrics/
A few options discussed before -> getsentry/sentry-java#1154

@stayallive
Copy link
Collaborator Author

Yeah good point, this was totally a placeholder name but something like runSpan and runTransaction already go way into SDK shorthands and those term might not necessarily mean anything to the end user (which I see as the primary consumer of this API) so I was looking for something that more speaks to what the user is getting out of it, like a measurement. But I must admit I do not have a better suggestion yet at this point 😄

Maybe it could be as simple as instrument since we usually describe how to do it under "Custom Instrumentation".

(also the performance metrics feature was unknown to me until now 👀)

Base automatically changed from 3.x to master October 18, 2022 15:22
@stayallive stayallive changed the title New Sentry\Laravel\measure custom span helper Introduce Sentry\Laravel\trace custom span helper Oct 20, 2022
// 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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this logic into the withScope call? This way we don't need to make the getCurrentHub call at the top.

Copy link
Collaborator Author

@stayallive stayallive Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we need to finish the span after the callback is called?

also - do we need tests?

Copy link
Collaborator Author

@stayallive stayallive Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@stayallive
Copy link
Collaborator Author

Proposal moved to getsentry/sentry-php#1490

@stayallive stayallive closed this Mar 10, 2023
@stayallive stayallive deleted the propose-span-helper branch March 10, 2023 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants