Improve RequestFetcher to work under RoadRunner

[joaopbnogueira]

Problem Statement

Right now RequestFetcher checks for \PHP_SAPI === 'cli', which will always be true if the application is running under RoadRunner even though all the necessary server headers with request information exist.

Right now, the workaround is to instantiate an instance of RequestIntegration with a custom RequestFetcher, but would be nice to not have to do it.

Solution Brainstorm

Possibly replace the current implementation with

    public function fetchRequest(): ?ServerRequestInterface
    {
        if (!isset($_SERVER['REQUEST_METHOD'])) {
            return null;
        }

        return ServerRequest::fromGlobals();
    }

but I'm not sure why \PHP_SAPI === 'cli' was added in the first place so this might cause other issues?

Thanks

[cleptric]

Thanks for bringing this up, we should get this fixed.

Using \PHP_SAPI === 'cli' is/was pretty common to check if a PHP script is being run in an CLI context. Times are changing though 😄

[joaopbnogueira]

Apologies for jumping to a solution this quickly, but it seems that this is not that simple.

When running under RoadRunner (PSR-7 compliant) we can't just do a ServerRequest::fromGlobals() as the request needs to be built using the appropriate PSR-17 factory HttpFactory when using Guzzle.

I'll looking into a potential solution for this.

[cleptric]

Sounds great, PRs are always welcome as well 🙂

[joaopbnogueira]

Ok, so I've got a working solution, but it's not pretty.

Since I'm processing requests in the context of a long-lived worker, that are created by a PSR-17 factory (so they can come from any source, e.g. file stream, RPC call, etc etc), the most natural approach that I could find is to re-initialize Sentry each time I get a new request.

This is less than ideal, as it would be nicer if there was a way to statically set the request (e.g. \Sentry\setRequest($request)) and then load it on the RequestFetcher for example.

Ugly solution:

public function handle(ServerRequestInterface $request): ResponseInterface
    {
        init([
            'environment' => $_ENV['ENVIRONMENT_NAME'] ?? $_ENV['ENVIRONMENT'] ?? 'unknown',
            'tags' => [
                'service' => $_ENV['SERVICE_NAME'],
            ],
            'integrations' => [new class($request) implements IntegrationInterface {

                private RequestIntegration $integration;

                public function __construct(?ServerRequestInterface $request)
                {
                    $this->integration = new RequestIntegration(new class($request) implements RequestFetcherInterface {
                        private ?ServerRequestInterface $request;

                        public function __construct(?ServerRequestInterface $request)
                        {
                            $this->request = $request;
                        }

                        public function fetchRequest(): ?ServerRequestInterface
                        {
                            return $this->request;
                        }
                    });
                }

                public function setupOnce(): void
                {
                    $this->integration->setupOnce();
                }

            }]
        ]);

        $this->reset();

        return $this->app->handle($request);
    }

[stayallive]

Statically setting the request can be achieved with a very small custom request fetcher of course. We implement one for Laravel too but there we get it from the container but that could be adapted to a static call too.

I'm not a 100% we should change our request integration by default because we probably have some other rough edges too with RoadRunner (like for example resetting the scope for each) and might need some extra help to fully make that easy to support and for normal cases is the SAPI name check a perfectly fine solution.

[Jean85]

Wouldn't a dedicated RoadRunner integration be a better approach? That would mean working with scopes too so that they would be popped between requests, as we do in Symfony with subrequests. This would not require re-initializing Sentry each time.

[joaopbnogueira]

Hi @Jean85 possibly yes, but RoadRunner is a go project that orchestrates workers and calls to PHP scripts through STDIN so it would be a different beast altogether no? Maybe we could get some ideas/hints on how to do it based on Laravel's Octane implementation.