Support http_connect_timeout and http_timeout options

[matthew-gill]

I created this issue in the Symfony repo: getsentry/sentry-symfony#592
This PR modifies the HttpClientFactory to allow configuration of the http (connect) timeout values.
If no option is set, it will use the default.

If I've missed the mark, feel free to reject - I'd just love to be able to do this in my app, if it's possible already please let me know, thanks!

[Jean85]

Apart from the explicit comments, this PR requires targeting develop, a changelog entry and some tests.

[matthew-gill]

Thanks @Jean85 for the suggestions/comments. I've made most of them.
Re tests, I was only able to write tests for the Options part.

I tried to do them for the HttpClientFactory but as the test harness doesn't have any real clients e.g. curl client or guzzle, I wasn't able to write tests without including the additional dependencies... they all use class_exists within HttpClientFactory::resolveClient

[matthew-gill]

I tried to write some tests by using class_alias in PHP (not something I've ever used before)! But in order to get it to work with the new PluginClient pieces, I'd either have to use reflection or add a getter to that class (to get the instanciated client), all of which I think feel a bit wrong for this case.

[Jean85]

Code seems fine to me, just a small comment and fixing PHPStan baseline is required.

[Jean85]

Looks fine to me 👍 great work!

In the meantime, I've checked with @bruno-garcia about the options naming, and he said that there's no standard, just Java SDK having sub-optimal names:
https://github.com/getsentry/sentry-java/blob/01cda06a94805f9940229e07ec7f31747f70474c/sentry/src/main/java/io/sentry/SentryOptions.java
https://github.com/getsentry/sentry-java/blob/01cda06a94805f9940229e07ec7f31747f70474c/sentry/src/main/java/io/sentry/SentryOptions.java

[Jean85]

For the CS issues, use make cs-fix to fix that automatically.

[mfb]

Looks like this didn't make it into 3.4.0 - but hopefully could make it into 3.5.0?

[matthew-gill]

Sorry for the radio silence on this, we've decided to use relay to alleviate our concerns. I'm afraid I don't have the bandwidth to pick this up at the moment, any volunteers to see this through would be greatly appreciated 🙏

[ste93cry]

hopefully could make it into 3.5.0

Sure, why not. I will try to find some time in the next days/weeks to finish the work. In the meanwhile, thank you @matthew-gill for having done most of it

[rabauss]

How can we support this PR to get it merged?

[mfb]

I'm curious what is the rationale of having http_timeout and http_connect_timeout options when the underlying Symfony HTTP client used by the SDK has max_duration and timeout options, which have different meanings. Wouldn't just having the same options be easier to grok?

[ste93cry]

the underlying Symfony HTTP client used by the SDK

The core SDK has no binding to any specific HTTP client, or at least we try to not have it... We provide some code to ease the intialization of the HTTP transport using some common clients, but this doesn't mean that we want to use them specifically besides that

[Jean85]

I discussed this privately with @ste93cry, but I'm still of the opinion that we should make these new options nullable and throw when we can't apply them, otherwise we could silently ignore them, and confuse the user.

So, I would change the check into something like:

if (
    null !== $options->getHttpProxy()
    || null !== $options->getHttpConnectTimeout()
    || null !== $options->getHttpTimeout()
    ) {
            throw new \RuntimeException('The "http_connect_timeout", "http_timeout" and "http_proxy" options are natively supported ony with "php-http/curl-client", "symfony/http-client" and "php-http/guzzle6-adapter"; either use one of them, or switch to build your HTTP transport manually.');
}

[ste93cry]

we should make these new options nullable and throw when we can't apply them, otherwise we could silently ignore them, and confuse the user.

While it's true that we could potentially confuse the user, I think that documenting the fact that these options could not apply to all HTTP clients would be enough. Making them nullable would confuse even more, as the user would not know what null means: does it means that a default applies (and what is it?), or that no timeout is set at all? Moreover, making the Options::getXXX() methods nullable just for the sake of checking elsewhere if those options apply to a particular client feels like a code smell

[Jean85]

Well, I think we need a tie breaker here... @stayallive? @smeubank? @HazAT?

[stayallive]

I think that making them nullable makes the most sense to be honest, because:

1. It's confusing when we have options that we can't apply, especially since they can have performance implications, so the user should know they are doing something that has no effect
2. The argument that making them nullable is confusing could be true, but we can easily document what null means here in the documentation, most people will never even configure these values anyway so having them nullable makes more sense I think. We've always had "hidden" default timeouts set so this is a great way to communicate that those are used and we can document where to find those values
3. Let's not try to get too hung up on perfectly architected code (whatever that definition is for you), in the end this client must report (potentially critical) errors to somewhere and must do it so with as little impact on the application as possible. It's goal is not to have perfectly architected code if that conflicts with the first 2 "goals" IMHO. Let's not forget: https://develop.sentry.dev/sdk/philosophy/#prioritize-customer-convenience-over-correctness.

[HazAT]

I am with @stayallive. Let's just document what null does (or doesn't do, meaning doesn't apply to all transports)
I wouldn't even throw an exception; if the http_client supports it, great; if it doesn't and you configured something manually, you probably anyway know what you are doing.

So I would say let's merge it.

More importantly, for me would be the change send_attempts to 0, not in this PR but referring to this comment #1311 (comment)

[ste93cry]

I wouldn't even throw an exception; if the http_client supports it, great; if it doesn't and you configured something manually, you probably anyway know what you are doing.

If we don't want to throw the exception, it makes little sense to have those options nullable. The whole argument in making them like so is because we want to throw an exception when the option could not apply to a transport, but as you said it makes little sense

More importantly, for me would be the change send_attempts to 0

This is a breaking change, people don't expect a default to change between minor versions

[stayallive]

There is no need to change send_attempts in this PR.

We do that separately and include that in a new major release of course instead of a minor.

[ste93cry]

I didn't mean that we need to change send_attempts in this PR, it was just a comment answering a side-topic. The point is that the only reason we were discussing to make the http_*_timeout options nullable is that we need to do it to be able to identify if the user set that option and throw an exception if he did and we can't autoconfigure the HTTP client. Since @HazAT agrees that it doesn't make sense to throw an error in that case, I cannot really understand why we want to keep going this way

[stayallive]

Don't think we are, as @HazAT mentioned we are going to document how they work but not going to add the exception in that case the defaults can live in the options 👍

This is how the PR currently is, I don't think there is anything to do, good to merge?