Closed voodooism closed 3 years ago
Thanks @voodooism, this would indeed be useful but I think can be done in a less obtrusive way (without the need for so many new check classes).
I think the liip_monitor.check
should have an optional label
attribute. In the CheckTagCompilerPass
:
Laminas\Diagnostics\Check\AbstractCheck
, call setLabel
when creating the service.If not set, decorate the check (using Symfony Service decoration) with a new LabelCheckDecorator
that extends Laminas\Diagnostics\Check\AbstractCheck
and then call setLabel
on it. This class would look like the following:
namespace Liip\MonitorBundle\Check;
use Laminas\Diagnostics\Check\AbstractCheck;
use Laminas\Diagnostics\Check\CheckInterface;
class LabelCheckDecorator extends AbstractCheck
{
private $inner;
public function __construct(CheckInterface $inner)
{
$this->inner = $inner;
}
public function check()
{
return $this->inner->check();
}
}
Sounds pretty much well. Okay, I'll implement it soon.
@kbond As it seems to me, there are several approaches.
Let's take an example of the php_extensions
. In the current implementation, we don't have any possibility to set a label explicitly
If I've understood correctly, you suggest something like that:
Configuration:
checks:
php_extensions:
extensions: [apc, xdebug],
label: 'My awesome label for all the extensions'
In that case, our bundle will instantiate Laminas\Diagnostics\Check\ExtensionLoaded and pass there the array containing all the extensions. It seems pretty much well, and we can set a label as you said above. I like this approach, but there is a little drawback: this label will apply to all the extensions.
I suggest doing it in such a way when we are able to set a personal label for every check (as I did in my PR).
checks:
php_extensions:
- apc
- { name: xdebug, label: 'My awesome label for only lovely xdebug' }
The same for other collections in the bundle. For instance: doctrine_dbal
check.
We can't just configure it something like that:
checks:
doctrine_dbal:
connections: [default, crm],
label: 'My awesome label for all the connections'
This feature makes sense only if we have an opportunity to set a personal label for every connection, like that:
checks:
doctrine_dbal:
- default // I don't care about the specific label here.
- { name: crm, label: 'My awesome label for CRM connection' }
In that case, I think it's not a very good idea to set labels in the compiler pass.
I try to say that we don't have a uniform configuration. In some cases like apc_memory
or even php_extensions
we definitely could just add label
attribute. But in cases like file_ini
or doctrine_dbal
, it seems not so easy.
What do you think about it? Speak your mind, please.
Ok, I see what you'd like to have now.
I recommend you define these checks manually, ie:
services:
check.xdebug:
class: Laminas\Diagnostics\Check\ExtensionLoaded
arguments: [xdebug]
calls:
- setLabel: ['My awesome label for only lovely xdebug']
tags: [liip_monitor.check]
A little verbose to be sure but I don't want to take on the maintenance burden of all the changes in this PR. I would accept a PR to add the optional tag label and LabelCheckDecorator
as described above. That would allow any check to be labelled (even if it doesn't extend AbstractCheck
) and simplify the manual service definition slightly:
services:
check.xdebug:
class: Laminas\Diagnostics\Check\ExtensionLoaded
arguments: [xdebug]
tags: [{ name: 'liip_monitor.check', label: 'My awesome label for only lovely xdebug' }]
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This pull request has been automatically closed. Feel free to re-create if it is still relevant.
The
expressions
check has a beautiful opportunity to set a custom label for every check. So why not add this for every LiipMonitorBundle check?In my projects, I use labels very often. It would be great to have an opportunity to set them using the only configuration.
I've added the
label
option for every check, except PDOCheck (because of this https://github.com/laminas/laminas-diagnostics/pull/14 ). And I also should notice that this feature doesn't break backward compatibility.