Sentry-php: Handling HttpAsyncClient in long living worker scripts

[mrtus]

We are using a lot of microservices, one disadvantage of that is that when using a library you do a lot of maintenance on them by keeping them up-to-date, yay for @dependabot.

But sometimes that can go wrong when you are only looking at which semantic version was bumped.

In https://github.com/getsentry/sentry-php/pull/981 there was a refactor introduced causing a breaking change when using EmulatedHttpAsyncClient to ensure sync requests are being made when an error occurs in a long-lived process worker.

My question is:
Can we come up with a proposal to make HttpSyncClient as a "default" factory implementation in this library without having to duplicate a custom DefaultTransportFactory ourselves numerous times?

Our custom TransportFactoryInterface implementation would look like this which is not very maintainable in my opinion:

return new class implements TransportFactoryInterface {
  public function create(Options $options): TransportInterface
  {
    $messageFactory = new GuzzleMessageFactory();

    $clientFactory = new HttpClientFactory(
      new GuzzleUriFactory(),
      $messageFactory,
      new GuzzleStreamFactory(),
      new EmulatedHttpAsyncClient(new Client()),
      SentryClient::SDK_IDENTIFIER,
      PrettyVersions::getVersion('sentry/sentry')->getPrettyVersion()
    );

    return new HttpTransport(
      $options,
      $clientFactory->create($options),
      $messageFactory,
      false,
      false
    );
  }
};

[ste93cry]

In #981 there was a refactor introduced causing a breaking change when using EmulatedHttpAsyncClient to ensure sync requests are being made when an error occurs in a long-lived process worker

We strive to not break BC, so if you found one please share the details as it was unintended :pray:

Our custom TransportFactoryInterface implementation would look like this which is not very maintainable in my opinion

I'm not really sure to get what in your opinion is not very maintainable. Regardless, you can use the ClientInterface::close method if you want to send the events right away without instantiating your own HttpTransport factory just to set the $delaySendingUntilShutdown flag to false. WDYT?

[mrtus]

@ste93cry Thank you for your quick response

We strive to not break BC, so if you found one please share the details as it was unintended 🙏

We have a lot of worker scripts, which did not send errors synchronously when they occurred, so we had to configure Sentry something like:

$client = ClientBuilder::create($context)
    ->setHttpClient(new EmulatedHttpAsyncClient(new Client()))
    ->getClient();

$hub = new Hub($client);

Hub::setCurrent($hub);

But due to the mentioned refactor (and thus the removal of the promises I guess) this no longer works for us. I realise setHttpClient is deprecated. So we ended implementing something like (still broken after the mentioned refactor):

$messageFactory = new GuzzleMessageFactory();

$transportFactory = new DefaultTransportFactory(
    $messageFactory,
    new HttpClientFactory(
        new GuzzleUriFactory(),
        $messageFactory,
        new GuzzleStreamFactory(),
        new EmulatedHttpAsyncClient(new Client()),
        SentryClient::SDK_IDENTIFIER,
        PrettyVersions::getVersion('sentry/sentry')->getPrettyVersion()
    )
);


$client = ClientBuilder::create($config)
    ->setTransportFactory(transportFactory)
    ->getClient();

I'm not really sure to get what in your opinion is not very maintainable.

Having to copy the mentioned snippet to every microservice, in my original question, is not maintainable, especially since it is leaking internal Sentry implementation details (in my opinion) such as

      SentryClient::SDK_IDENTIFIER,
      PrettyVersions::getVersion('sentry/sentry')->getPrettyVersion()

But what is maintainable and what isn't, is probably a personal opinion and is maybe not something we should put into a discussion here and now, we can do that somewhere else if necessary 😆.

My point was mainly, that if this implementation wants to offer both Sync & Async transports maybe it should be very easy to implement by plug-and-play, but reading the next quote, I think we have been missing something.

Regardless, you can use the ClientInterface::close method if you want to send the events right away without instantiating
your own HttpTransport factory just to set the $delaySendingUntilShutdown flag to false. WDYT?

I do not seem to find a ClientInterface::close method.

Maybe an example could do? How would you adapt the following snippet (Using version sentry/sentry:2.3.2) to ensure that every error that occurs is logged synchronously to Sentry instantly without shutting down the long-running script?

$client = ClientBuilder::create($context)
    ->setHttpClient(new EmulatedHttpAsyncClient(new Client()))
    ->getClient();

$hub = new Hub($client);

Hub::setCurrent($hub);

Maybe I am missing something very obvious here.

[ste93cry]

I do not seem to find a ClientInterface::close method.

Pardon, the method I meant to write is FlushableClientInterface::flush, which is implemented in the default SDK client: by calling it you will force the pending events to be sent right away. I imagine that the worker script is basically an infinite loop, so you could call that method at the end of each iteration to batch sending before starting the next one. The alternative is what you already did: instantiate your own transport that uses a syncronous HTTP client: this will be the default in the next major version of the library.

if this implementation wants to offer both Sync & Async transports it should be very easy to implement

Indeed we would like to support both yncronous and asyncronous transports, but the latter due to how PHP works are more difficult to get implemented right and usually requires a full event-loop to be used to not block other things (e.g. reactphp/event-loop). This is why for simplicity we would like to switch to a full syncronous transport out-of-the-box in the next major version

especially since it is leaking internal Sentry implementation details

The server requires those information (even though imho it shouldn't) in the authentication process, but you can set them to whatever data you want because I think it's used only for gathering statistics on the SaaS service. I don't have any real other solution to simplify more than this setting a custom HTTP client, but if you do feel free to share it

[mrtus]

The alternative is what you already did: instantiate your own transport that uses a syncronous HTTP client: this will be the default in the next major version of the library.

Any idea when the next major would be released?

And about https://github.com/getsentry/sentry-php/pull/981 since it is kinda breaking things, can it be fixed so the previous implementation, through setting an emulated client, can work again?

$client = ClientBuilder::create($context)
    ->setHttpClient(new EmulatedHttpAsyncClient(new Client()))
    ->getClient();

$hub = new Hub($client);

Hub::setCurrent($hub);

I guess we'll have to stick on sentry/sentry:2.3.1 for now then.

[ste93cry]

Any idea when the next major would be released?

No, we didn't even start its development and I have no ETA on when we will do it

And about #981 since it is kinda breaking things, can it be fixed so the previous implementation, through setting an emulated client, can work again?

The right answer would be yes, but what seems to be a BC really is a bug fix so I don't think we can and want to revert it as it wasn't working as intended before. I don't really have a workaround to provide to make it working as you want though, but I'm open to discuss potential ideas

[mrtus]

Okay, I'm not too familiar with the implementation of this library so after a quick look this would be my proposal: introduce a builder method that makes it easy to enable the synchronous transport and a DefaultSynchronousTransportFactory which sets the flag of the transport to false:

ClientBuilder::create($context)
  ->enableSynchronousTransport()
  ->getClient();

Internally this builder would have a flag and depending on the flag state a different factory would be used in the createDefaultTransportFactory

Something like:

private function createDefaultTransportFactory(): DefaultTransportFactory
{
  $this->messageFactory = $this->messageFactory ?? MessageFactoryDiscovery::find();
  $this->uriFactory = $this->uriFactory ?? UriFactoryDiscovery::find();
  $this->streamFactory = $this->streamFactory ?? StreamFactoryDiscovery::find();

  $httpClientFactory = new HttpClientFactory(
    $this->uriFactory,
    $this->messageFactory,
    $this->streamFactory,
    $this->httpClient,
    $this->sdkIdentifier,
    $this->sdkVersion
  );

  if (!empty($this->httpClientPlugins)) {
    $httpClientFactory = new PluggableHttpClientFactory($httpClientFactory, $this->httpClientPlugins);
  }

  if($this->enableSynchronousTransport){
    return new DefaultSynchronousTransportFactory($this->messageFactory, $httpClientFactory);
  }

  return new DefaultTransportFactory($this->messageFactory, $httpClientFactory);
}

This change is not breaking and it moves the responsibility of configuring the async-sync transport to the builder (more or less)

Would this solution work for you?

I realise this might be very use-case specific, but if I read your comments on other issues correctly it was never the intension to have async transport by default, this small change would move into that direction to make it very easy to use synchronous transport through configuration on the builder.

[tm1000]

Re:

Pardon, the method I meant to write is FlushableClientInterface::flush, which is implemented in the default SDK client: by calling it you will force the pending events to be sent right away.

The reference for that is:

\Sentry\SentrySdk::getCurrentHub()->getClient()->flush();

Luckily the system I am using catches all exceptions so my code ends up being (putting this here so others looking on how to access flush() can figure out how to use it)

\Sentry\captureException($exception);
\Sentry\SentrySdk::getCurrentHub()->getClient()->flush();

[mrtus]

@ste93cry If I'm correct, this async issue has now been resolved in version 3 by this PR? https://github.com/getsentry/sentry-php/pull/1066, so long running scripts will now send their errors sync to Sentry without delay right? I atleast see that the boolean has been removed https://github.com/getsentry/sentry-php/pull/1066/files#diff-04acbfe90db20752c46daaa8ffaea0abL80

If so you may close this issue 😃 .

[ste93cry]

this async issue has now been resolved in version 3 by this PR?

Correct, in 3.0 we switched the HTTP transport to be syncronous. Please note that this means that the event will be sent right away also in the context of an HTTP request, they will not be sent after the response has been sent as it was happening before. This may hurt performance and page speed a lot, so the downside is that for the moment if you want this kind of behavior you will have to write your own transport implementation

[mrtus]

@teamleader We are all good with synchronous transports for errors. Thank you for the swift update!