Sentry-php: Make sending behavior of the HttpTransport "delaySendingUntilShutdown" configurable (RFC)

[bzannah]

Clearly writing custom transport is not something as easy as it seems right now since you have to configure yourself the HTTP client with all the plugins needed for authentication.

Also since the behavior of the HttpTransport by sending events immediately instead of waiting the shutdown of the application is deprecated and has a significant impact on performance.

For applications with use cases where there is a need to send events immediately regardless of performance, this is a big dilemma. Although it is possible using a compiler pass or an extension that accesses the transport properties using reflection, the solution may seem hacky and perhaps unnecessary.

Therefore the best outcome could be to give make this delay configurable so anyone can use it the way they want.

[mfb]

As a Sentry user, this seems like a reasonable config - although every config does add overhead to the library in terms of maintainability, testing etc.

Sending events in parallel at shutdown or when flush() is called would be a reasonable default for me, since some of my apps send multiple events to Sentry fairly often and I need to minimize execution time. But other users might prefer to send events immediately, depending on their use case.

[Jean85]

FTR, this was already explained by @ste93cry in https://github.com/getsentry/sentry-php/issues/811#issuecomment-490066559 (and probably elsewhere too).

[ste93cry]

Clearly writing custom transport is not something as easy as it seems right now since you have to configure yourself the HTTP client with all the plugins needed for authentication.

This is going to change with version 2.3. You will be able to customize the HTTP client without worring about which plugins are required to make it working with Sentry

Also since the behavior of the HttpTransport by sending events immediately instead of waiting the shutdown of the application is deprecated and has a significant impact on performance.

The next patch release is going to rollback the change of the default behavior of sending, however as explained in the comment @Jean85 linked above the deprecation will remain. This however does not means that we cannot think about making a new transport that sends events in parallel, asyncronously or whatever we think is appropriate

[nenadalm]

So as I understand it, logs were not meant to be sent when request ends and there will be some work on it. Good. I am just checking how to enable sentry in long running process (probably overwriting the delaySendingUntilShutdown via reflection).

Writing just to add the reason why one might want to send logs earlier (ideally that process would run without crash until new version of the app is available).

[ste93cry]

So as I understand it, logs were not meant to be sent when request ends and there will be some work on it.

This is exact. What I had in mind when I coded such transport was that events were to be sent as soon as they were emitted, asyncronously or sincronously. In the asyncronous case, to ensure that everything was sent before the shutdown of the application we run the cleanupPendingRequests method that basically awaits for each promise to return a result, thus effectively blocking the program, but this was not the main reason I added such method. Of course, in the PHP world if you are not integrating the promises with a real event loop then it will always block execution.

Good. I am just checking how to enable sentry in long running process (probably overwriting the delaySendingUntilShutdown via reflection).

You should not use reflection for this. That flag is a parameter that you can configure, but using delaySendingUntilShutdown set to true you must be conscious that you are relying on a deprecated behavior that will stop working in version 3.0. If you're fine with this is up to you. If you don't want to take this responsibility you should leave the flag off and instead call manually the flush method on the client each time you feel you need it so that the queue of events to be sent will drain. Again, if the underlying HTTP transport (and so what handles the promises) is not async then you will block the execution of the program until the HTTP request ends

[rbaarsma]

I've read all comments above, but still don't understand how to turn this off? Is there a version I should downgrade to?

I'm using monolog and the BufferHandler to send a single sentry event, which uses the register_shutdown_function too, to eventually send the logs.

Unfortunately due to delaySendingUntilShutdown being true in the ClientBuilder (line 348), I don't really see how to fiix this, except to create my own ClientBuilder, which I would prefer not to do.

Is there an older version to fall back to? Or is there a better way to create a client? How should I fix this?

[nenadalm]

@rbaarsma I also didn't want to create custom ClientBuilder and since I am using Symfony, I just overwrote the parameter via reflection on kernel boot :).

    public function boot(): void
    {
        parent::boot();
        // `Sentry` is just helper class that sets the property and `app.monolog.sentry_client` is alias to
        // `Sentry\ClientInterface`
        Sentry::fixClient($this->getContainer()->get('app.monolog.sentry_client'));
    }

[rbaarsma]

@nenadalm thanks for your answer

I eventually chose to revert back to a working version of sentry + monolog-sentry-handler in my composer.json

        "bgalati/monolog-sentry-handler": "1.1.0",
        "sentry/sentry": "2.2.2",

[B-Galati]

@ste93cry

Of course, in the PHP world if you are not integrating the promises with a real event loop then it will always block execution.

curl-ext is able to do it in a non blocking way without event loop. Guzzle and Symfony Http Client are leveraging this feature.

[ste93cry]

Again, this is not asyncronous behavior... Asyncronous behavior CANNOT be replicated without an event loop. I don't know about Symfony HTTP client, but Guzzle has a queue that must be ticked to advance the request, and ticking that request is a syncronous operation that will block the execution. What cURL can do is sending multiple requests in parallel, but it doesn't mean that doing it the execution of the program will continue as you would expect for example in JS 😉

[Jean85]

The docs declare full async: https://symfony.com/doc/current/components/http_client.html#making-requests

I don't know how they do it BTW.

[tbickley-mediabowl]

I just want to confirm that I'm understanding this situation correctly, because it seems really bizarre.

The behaviour to delay sending events until shutdown is deprecated, but at the same time it is also the default behaviour, and there is no option to disable it.

And so the only way to _not_ use this deprecated behaviour (until it's removed in 3.0) is to do something with reflection along the lines of:

$client = Sentry\SentrySdk::getCurrentHub()->getClient();
if ($client instanceof Sentry\Client) {
    try {
        $clientReflection = new ReflectionClass(Sentry\Client::class);

        $clientTransportProperty = $clientReflection->getProperty('transport');
        $clientTransportProperty->setAccessible(true);

        $transport = $clientTransportProperty->getValue($client);
        if ($transport instanceof Sentry\Transport\HttpTransport) {
            $transportReflection = new ReflectionClass(Sentry\Transport\HttpTransport::class);

            $transportDelaySendingUntilShutdownProperty = $transportReflection->getProperty('delaySendingUntilShutdown');
            $transportDelaySendingUntilShutdownProperty->setAccessible(true);

            $transportDelaySendingUntilShutdownProperty->setValue($transport, false);
        }
    } catch (ReflectionException $e) {
        error_log($e);
    }
}

Is all of that correct?

[ste93cry]

The behaviour to delay sending events until shutdown is deprecated, but at the same time it is also the default behaviour, and there is no option to disable it.

The behavior is deprecated, but must still be the default until 3.0 to not break people's applications. To stop using the deprecated behavior you need to instantiate yourself the HttpTransport transport using the client builder and configure the $delaySendingUntilShutdown argument of the constructor. Note that no deprecation error should be thrown if you simply call \Sentry\init(), so you can simply not bother about this and wait for the switch in the next major version 😉

[robjr]

I'm also facing the same issue :/ This delay could be made configurable with default true so that we wouldn't have to implement this hacky way until the 3.0 finally comes out.

[ste93cry]

As I see a lot of confusion regarding this issue, I will try to make things more clear: the fact that the events are sent at the shutdown of the application rather than immediatly is something that is considered deprecated for the HttpTransport (which is the only HTTP transport provided out-of-the-box) and won't work in 3.0. Nothing prevents us, if people think that it's useful, to provide in that version another transport that still sends events at the shutdown, but it won't be supported for sure by the HttpTransport because this transport, as the name implies, it syncronous. At the time of writing though, to avoid breaking all existing applications, no deprecation error is thrown if you call \Sentry\init() or you use a framework's integration without customizing the initialization of the SDK and I think that this is the common use case out of here. If you want to switch behavior to be ready for the next major version you can by toggling the flag of the constructor of such transport, but this is something that you must do on your own because you decide that you want to do it.

[tbickley-mediabowl]

The thing I found confusing is that there's no easy way to switch to the non-deprecated way of doing things. You keep saying about toggling the flag in the constructor, but that call is three classes deep into the init method that I assume most people are using, meaning that we have to dig into the library and duplicate a bunch of its code (or use Reflection, which seems simpler and therefore more sensible for temporary code) in order to use the library the way it's supposed to work in the first place.

I have no objection to the change needing to be conscious choice to avoid breaking things, but it seems weird that it's expected that people will go and write a bunch of temporary code just to be able to use this library as it's intended. Really feels like there should just be a flag we can pass into the main init method or something...

[ste93cry]

meaning that we have to dig into the library and duplicate a bunch of its code (or use Reflection, which seems simpler and therefore more sensible for temporary code) in order to use the library the way it's supposed to work in the first place

First of all, you should never use reflection to access private things of a library you're using or as I'm pretty sure you know your code may suddenly cease to work, and if you need to do it either there is something wrong or you are taking this path consciously accepting the risks it implies. In any case, even for temporary code (how much temporary? There is no ETA for 3.0...), I highly recommend to avoid using reflection to change such things. Anyway, the code below should do exactly what you did with reflection, but using the expected public API to do this kind of things:

$client = ClientBuilder::create([...])
    ->setTransportFactory(new class implements TransportFactoryInterface {
        public function create(): TransportFactoryInterface
        {
            return new HttpTransport(...);
        }
    })
    ->getClient();

I wrote the code on the fly, so it may need some adjustments, but it should also be enough to understand the idea behind it.

[tbickley-mediabowl]

Trying to do it this way, we don't have access to two of the parameter values passed in to the constructor of HttpTransport: $httpClient and $requestFactory. In the existing code the $httpClient is constructed using six private properties of ClientBuilder, and the messageFactory is itself a private property of ClientBuilder, so we don't have access to any of these.

I'm not saying reflection is an ideal solution, but at the moment it seems like the only option we have (and the chance of anything breaking seems fairly slim - would basically only be if the Client::transport or HttpTransport::delaySendingUntilShutdown properties got renamed as far as I can see)

[ste93cry]

In the existing code the $httpClient is constructed using six private properties of ClientBuilder, and the messageFactory is itself a private property of ClientBuilder, so we don't have access to any of these.

Those properties are all deprecated (I will add the missing annotations in case it helps make this clearer, the setters are already triggering the deprecation error), so I discourage relying in them. What we trying to achieve is stop leaking dependencies of a specific transport implementation into the client builder: accepting to add a new flag in the builder means going against this intention. If you want to instantiate the HttpTransport then it's up to you to also instantiate its dependencies, which include the HTTP client. In turn, I think that if you want to do this then you are falling into an advanced use-case and thus you should not be scared of writing more code to make things work as you wish.

[ccnixon]

If it helps anyone, I basically just copy/pasted the example in the docs and it _appears_ to be working. I haven't done a ton of testing yet though:

/** @var array|Sentry\Options */
$options = [
    'dsn' => getenv('SENTRY_DSN'),
    'release' => $_ENV['RELEASE_VERSION'] ?? null,
];

\Sentry\init($options);

$httpClientFactory = new HttpClientFactory(
    UriFactoryDiscovery::find(),
    MessageFactoryDiscovery::find(),
    StreamFactoryDiscovery::find(),
    HttpAsyncClientDiscovery::find(),
    'sentry.php',
    '2.3'
);

$transportFactory = new class($httpClientFactory) implements TransportFactoryInterface {
    /**
     * @var HttpClientFactoryInterface
     */
    private $clientFactory;

    public function __construct(HttpClientFactoryInterface $clientFactory)
    {
        $this->clientFactory = $clientFactory;
    }

    public function create(\Sentry\Options $options): TransportInterface
    {
        // The last argument is where we disable the queuing behavior.
        return new HttpTransport(
            $options,
            $this->clientFactory->create($options),
            MessageFactoryDiscovery::find(),
            false
        );
    }
};

$builder = ClientBuilder::create($options);
$builder->setTransportFactory($transportFactory);
Hub::getCurrent()->bindClient($builder->getClient());

It seems pretty verbose to flip a bit but 🤷