Add Graphite Web HTTP request timeout option

[atj]

If the Graphite Web server is unreachable, all requests for frontend pages containing graphs hang until the backend HTTP request times out, resulting in a very poor UX.

The Guzzle documentation states that the default behaviour is to wait indefinitely, however in our testing the cURL handler has an internal default of 30 seconds:

https://docs.guzzlephp.org/en/stable/request-options.html#timeout

This commit makes the HTTP request timeout configurable and sets a reasonable default of 10 seconds.

[nilmerg]

Thanks!

I've just left you some suggestions to solve two problems:

• The type safety as reported by phpstan
• The fallback for configs without a timeout, so that the new default of 10 seconds applies to them as well

[atj]

@nilmerg Thanks for the suggestions. I had tried to fix the type safety issues reported by PHPStan but couldn't understand why the following calls weren't triggering the same errors:

icingaweb2-module-graphite/library/Graphite/Graphing/GraphingTrait.php

Lines 71 to 73 in 254ff35

	->setUser($graphite->user)
	->setPassword($graphite->password)
	->setInsecure($graphite->insecure)

I applied the suggestions locally and PHPStan raised the same error with a slightly different expected type:

$ vendor/bin/phpstan analyse library/ application/
Note: Using configuration file icingaweb2-module-graphite/phpstan.neon.
 42/42 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ -------------------------------------------------------------------------------------------------------------------------------- 
  Line   library/Graphite/Graphing/GraphingTrait.php (in context of class Icinga\Module\Graphite\Clicommands\Icinga2Command)             
 ------ -------------------------------------------------------------------------------------------------------------------------------- 
  :74    Parameter #1 $timeout of method Icinga\Module\Graphite\Graphing\GraphiteWebClient::setTimeout() expects int|null, mixed given.  
 ------ -------------------------------------------------------------------------------------------------------------------------------- 

[nilmerg]

The problem is that the config section type has no idea of its contained properties and hence phpstan complains rightfully about the unsafe handling. The config might as well contain timeout = "20" which is a string.

To fix this, I'd suggest to use this in GraphingTrait:

setTimeout(isset($graphite->timeout) ? (int) $graphite->timeout : null)

[nilmerg]

Extending the baseline isn't an option.

[atj]

Extending the baseline isn't an option.

PHPStan is not throwing type errors for the other config fields because they are being ignored via entries in the baseline:

icingaweb2-module-graphite/phpstan-baseline.neon

Lines 68 to 76 in 254ff35

	-
	message: "#^Parameter \\#1 \\$insecure of method Icinga\\\\Module\\\\Graphite\\\\Graphing\\\\GraphiteWebClient\\:\\:setInsecure\\(\\) expects bool, mixed given\\.$#"
	count: 1
	path: application/controllers/GraphController.php
	-
	message: "#^Parameter \\#1 \\$password of method Icinga\\\\Module\\\\Graphite\\\\Graphing\\\\GraphiteWebClient\\:\\:setPassword\\(\\) expects string\\|null, mixed given\\.$#"
	count: 1
	path: application/controllers/GraphController.php

As the Icinga\Data\ConfigObject accessors are returning mixed values, I don't believe it's possible to make the call to setTimeout type safe, so it's the only option.

[nilmerg]

If you rebase, phpstan shouldn't complain anymore.

We've introduced phpstan to our CI toolstack just recently and didn't adjust most of what it complained about. Hence why the baselines are rather large at the moment. Though, adding additional entries to those isn't helpful in any way as it renders phpstan useless after all.

I've played around with generics just recently and now added them to Icinga Web's ConfigObject. This allows us to define here what our config contains. I've did that now in the default branch and you don't have to do anything, other than rebasing. So please drop the changes in the baseline.

[atj]

Thanks for your help on this. I've removed the baseline changes and ensured that Guzzle isn't passed a null value for timeout.