Allow use of current hub scope in Messenger Listener

[sh41]

Hopefully this is the right way to do things. Thanks to @emarref for the Messenger listener - I had no idea how much I was missing from these exceptions being swallowed. I would like to add another listener with a higher priority that adds user details to the scope, but found that there was no way to access the scope of the Client in the new listener. I've changed the listener so that it uses the currenthub. This allows me to create a higher priority listener that will populate the scope with something like the below.

<?php

namespace App\EventListener;

use App\Repository\UserRepository;
use Sentry\SentrySdk;
use Sentry\State\Scope;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent;

class SentryMessengerListener implements EventSubscriberInterface
{
    /**
     * @var bool
     */
    private $captureSoftFails;
    /**
     * @var UserRepository
     */
    private UserRepository $userRepository;

    /**
     * @param UserRepository $userRepository
     * @param bool $captureSoftFails
     */
    public function __construct(UserRepository $userRepository, bool $captureSoftFails = true)
    {
        $this->userRepository = $userRepository;
        $this->captureSoftFails = $captureSoftFails;
    }

    /**
     * @param WorkerMessageFailedEvent $event
     */
    public function onWorkerMessageFailed(WorkerMessageFailedEvent $event): void
    {
        if (! $this->captureSoftFails && $event->willRetry()) {
            // Don't capture soft fails. I.e. those that will be scheduled for retry.
            return;
        }
        // If hte message has a userId, look up the user and add it to the current hub's scope.
        $message = $event->getEnvelope()->getMessage();
        $userId = $message->userId??null;
        if (!$userId) {
            return;
        }
        $user = $this->userRepository->find($userId);
        if ($user) {
            $userData = [
                'username' => $user->getUsername(),
            ];
            SentrySdk::getCurrentHub()
                ->configureScope(
                    function (Scope $scope) use ($userData): void {
                        $scope->setUser($userData, true);
                    }
                );
        }
    }

    /**
     * @return array
     */
    public static function getSubscribedEvents()
    {
        return [
            WorkerMessageFailedEvent::class => [ 'onWorkerMessageFailed', 200 ],
        ];
    }
}

I think I've got all of the tests working. Open to feedback, or another way of accessing scope for the listeners. Please just let me know.

[emarref]

Thanks heaps for this update. It certainly is nice to add this functionality I'd missed.

My only question is if it would be possible to inject the hub instead of acquiring it via static access.

[emarref]

For your local high-priority listener, why not configure the scope as soon as you know the user, rather than only when capturing exceptions? This would also help when pushing information to Sentry that is not an exception. E.g. breadcrumbs, or manual error captures.

Perhaps this is personal preference coming through, but I would be inclined to "select" the user somehow that makes sense to your app. E.g. in some service, which fires an event alerting the app. You'd then have a listener on that "user selected" event that configures the Sentry scope.

[Jean85]

I'm not sure what you're trying to achieve here, but I've reviewed it nevertheless.. I hope that you either get enough info from the review to solve your problem otherwise, or we can agree on how to proceed here! 👍

[Jean85]

Also, please check #340.

[sh41]

Also, please check #340.

@Jean85 What's the best approach with #340? Should I wait until it has been approved and then rework this PR around it? It is all very much in the same area.

[sh41]

For your local high-priority listener, why not configure the scope as soon as you know the user, rather than only when capturing exceptions? This would also help when pushing information to Sentry that is not an exception. E.g. breadcrumbs, or manual error captures.

Perhaps this is personal preference coming through, but I would be inclined to "select" the user somehow that makes sense to your app. E.g. in some service, which fires an event alerting the app. You'd then have a listener on that "user selected" event that configures the Sentry scope.

@emarref I had been thinking of adding to the scope using Middleware rather than an event listener. My consumers all run asyncronously on different VMs to the message producers, so the only information for scope that I have comes from the message itself. There is probably a clever way to add information via stamps and then use those stamps in some middleware to populate scope for a particular message.

[emarref]

Yeah sounds like middleware is the way to go for your use case. Just remember that your middleware should unset what it adds to the scope after the command has been handled or if it fails.

[Jean85]

Yeah sounds like middleware is the way to go for your use case. Just remember that your middleware should unset what it adds to the scope after the command has been handled or if it fails.

Unset is overkill, you could just Scope::push at the start and Scope::pop at the end, and you're good to go. Push adds a new scope to the stack with a copy of all the preexisting data, pop removes that scope from the stack, basically doing a full undo.

@Jean85 What's the best approach with #340? Should I wait until it has been approved and then rework this PR around it? It is all very much in the same area.

Due to my response above ⬆️ I think that you should wait for #340; with that plus those advices you'll probably be able to do everything you need. Ping me if you need more suggestions.

[sh41]

@Jean85 Thanks, I've merged in the changes from #340. I believe that this PR is still valuable because without it the hub's scope is not included in any exception reported back to Sentry. By changing the listener to use HubInterface::captureException rather than FlushableClientInterface::captureException any scope that has been added to the hub will be sent with the flush. I can't see any other way of adding scope, but happy to be steered right if I am mistaken with that?

[Jean85]

No you're right, pushing calls through the Hub is better that going through the client, which is more level. The error comes probably from the old SDK approach, where the Client was a god-class that did everything, from tracing data to sending stuff over HTTP.

LGTM! Thank you for the PR! 👍

[sh41]

Thanks @Jean85 & thanks again to @emarref for the initial PR. I have found so many bugs that we just couldn't see before.

For the benefit of anyone looking to add UserContext to their Messenger exceptions, I have ended up with middleware to configure the scope. It uses a stamp to keep the user details with the envelope. If there is currently a logged in user it will stamp the envelope appropriately. Alternatively in a situation where I don't have logged in user I can use something like this:

$bus->dispatch(new Message(), [new UserContextStamp($userId, $username)]);

These are my Stamp and Middleware classes. Feel free to use. YMMV.

namespace App\Messenger\Stamp;

use Symfony\Component\Messenger\Stamp\StampInterface;

class UserContextStamp implements StampInterface
{
    public int $id;
    public string $username;

    public function __construct(int $id, string $username)
    {
        $this->id = $id;
        $this->username = $username;
    }
}

namespace App\Messenger\Middleware;

use App\Messenger\Stamp\UserContextStamp;
use Sentry\State\HubInterface;
use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Messenger\Middleware\MiddlewareInterface;
use Symfony\Component\Messenger\Middleware\StackInterface;
use Symfony\Component\Messenger\Stamp\HandledStamp;
use Symfony\Component\Security\Core\Security;
use Symfony\Component\Security\Core\User\UserInterface;

final class SentryUserContextMiddleware implements MiddlewareInterface
{
    /**
     * @var HubInterface
     */
    private HubInterface $hub;
    /**
     * @var Security
     */
    private Security $security;

    /**
     * @param HubInterface $hub
     * @param Security $security
     */
    public function __construct(HubInterface $hub, Security $security)
    {
        $this->hub = $hub;
        $this->security = $security;
    }

    public function handle(Envelope $envelope, StackInterface $stack): Envelope
    {
        // If we haven't previously had a UserContextStamp added and Security is
        // able to provide a suitable user, add it to the envelope.
        if (null === $envelope->last(UserContextStamp::class)) {
            $user = $this->security->getUser();
            if ($user instanceof UserInterface) {
                $envelope = $envelope->with(new UserContextStamp($user->getId(), $user->getUsername()));
            }
        }

        // Push a new scope and then configure it with the user context if we have one.
        $scope = $this->hub->pushScope();

        // The stamp might have just been added above, or might have been added during dispatch,
        // in other middleware or any other time before now.
        /** @var $usernameStamp UserContextStamp */
        if (null !== ($usernameStamp = $envelope->last(UserContextStamp::class))) {
            $userContext['id'] = $usernameStamp->id;
            $userContext['username'] = $usernameStamp->username;
            $scope->setUser($userContext, true);
        }
        try {
            $envelope = $stack->next()->handle($envelope, $stack);
            return $envelope;
        } finally {
            // if we successfully handled the message we can clear the scope
            if (null !== $envelope->last(HandledStamp::class)) {
                // pop scope so that the hub is all reset ready for the next message coming through.
                $this->hub->popScope();
            }
        }
    }
}

There is a slight inefficiency with this, which I'm not sure how to solve, which is that the new MessengerListener only triggers after the entire middleware stack has finished executing. That means that if I want my scope to still be in the hub's stack when a WorkerMessageFailed event is fired I can't pop it. That's what the finally part of the middleware above is for. If the message was properly handled we can lose the scope, but if it is not properly handled we need to keep it for the MessengerListener call which comes afterwards. I think I might need to add a lower priority listener for WorkerMessageFailed to pop the scope.

[emarref]

the new MessengerListener only triggers after the entire middleware stack has finished executing

True.

However, the danger with this approach is that your scope will never be popped for failed messages. Subsequent messages failing on the same worker will get deeper and deeper in scopes. Unless you have a custom listener on the handler failed event to pop the scope. But this seems disconnected and messy.

Perhaps a better approach instead of middleware is a custom subscriber in your app that listens to the WorkerMessageReceivedEvent event to grab the UserContextStamp and push the scope and configure it with user information. You can then pop the scope by listening for the WorkerMessageFailedEvent and WorkerMessageHandledEvent events.

In fact, it might even be something that could be added to the bundle.

[Jean85]

In fact, it might even be something that could be added to the bundle.

I agree 👍 Is it possible to push/pop around a single handler too or there is no event to listen in that case?

[emarref]

Is it possible to push/pop around a single handler too or there is no event to listen in that case?

There is no event, so I don't think there is a way to do this.

https://github.com/symfony/messenger/blob/master/Middleware/HandleMessageMiddleware.php#L56-L69

[sh41]

I dropped the Middleware in favour of an event based system. The below is what I have right now. The SendMessageToTransportsEvent is used on the producer side to extract user information from the security service if there isn't already a UserContextStamp attached.

On my async worker side of things the scope is populated from the details in the stamp as soon as the message is received and then the scope is popped when the message processing completes, either by a successful handling or by a failure.

Edited: Changed the priorities to add context before all other listeners and remove it after all other listeners. Means that a failure anywhere in the pipeline of listeners and middleware will have the best chance of having context.

namespace App\EventListener;

use App\Messenger\Stamp\UserContextStamp;
use Sentry\State\HubInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Messenger\Event\SendMessageToTransportsEvent;
use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent;
use Symfony\Component\Messenger\Event\WorkerMessageHandledEvent;
use Symfony\Component\Messenger\Event\WorkerMessageReceivedEvent;
use Symfony\Component\Security\Core\Security;
use Symfony\Component\Security\Core\User\UserInterface;

class AddSentryContextToMessengerListener implements EventSubscriberInterface
{
    /**
     * @var HubInterface
     */
    private HubInterface $hub;
    /**
     * @var Security
     */
    private Security $security;

    /**
     * @param HubInterface $hub
     * @param Security $security
     */
    public function __construct(HubInterface $hub, Security $security)
    {
        $this->hub      = $hub;
        $this->security = $security;
    }

    public function onSendMessageToTransports(SendMessageToTransportsEvent $event)
    {
        $envelope = $event->getEnvelope();
        // If we haven't previously had a UserContextStamp added and Security is
        // able to provide a suitable user, add it to the envelope.
        if (null === $envelope->last(UserContextStamp::class)) {
            $user = $this->security->getUser();
            if ($user instanceof UserInterface) {
                $envelope = $envelope->with(new UserContextStamp($user->getId(), $user->getUsername()));
                $event->setEnvelope($envelope);
            }
        }
    }

    public function onWorkerMessageReceived(WorkerMessageReceivedEvent $event)
    {
        $envelope = $event->getEnvelope();
        // Push a new scope and then configure it with the user context if we have one.
        // we alway push a new scope, because we always pop the scope on completion.
        $scope = $this->hub->pushScope();

        // The stamp might have just been added above, or might have been added during dispatch,
        // or any other time before now.
        /** @var $usernameStamp UserContextStamp */
        if (null !== ($usernameStamp = $envelope->last(UserContextStamp::class))) {
            $userContext['id']       = $usernameStamp->id;
            $userContext['username'] = $usernameStamp->username;
            $scope->setUser($userContext, true);
        }
    }

    public function onWorkerMessageFailedOrHandled(): void
    {
        // runs after the built in listener to clear the scope.
        $this->hub->popScope();
    }

    /**
     * @return array
     */
    public static function getSubscribedEvents()
    {
        return [
            SendMessageToTransportsEvent::class => ['onSendMessageToTransports,128'],
            WorkerMessageReceivedEvent::class   => ['onWorkerMessageReceived',128],
            WorkerMessageFailedEvent::class     => ['onWorkerMessageFailedOrHandled', -127],
            WorkerMessageHandledEvent::class    => ['onWorkerMessageFailedOrHandled', -127],
        ];
    }
}

[emarref]

Looks good