Execution event for Open and Close circuit breaker, getMetrics() method visibilitiy.

[ogolubenco]

Hi, i have 2 questions:

1) Is it possible to add a single execution event on circuit open and circuit close ? right before calling $this->stateStorage->openCircuit() and $this->stateStorage->closeCircuit() . 2) Is it possible to change AbstractCommand->getMetrics() method visibility to public ?

I'll explain what i'm trying to achieve based on this example:

<?php

declare(strict_types = 1);

namespace Path\To\Folder;

use Odesk\Phystrix\AbstractCommand;
use Odesk\Phystrix\MetricsCounter;
use Psr\Log\LoggerInterface;

class TestCommand extends AbstractCommand
{
    private $client;
    private $logger;

    public function __construct($client, LoggerInterface $logger) {
        $this->client = $client;
    }

    protected function run()
    {
        return $this->client->doSomething();
    }

    protected function processExecutionEvent($eventName) : void
    {
        if ($eventName === self::EVENT_SUCCESS) {
            // Log all successful calls to the client
            $this->logger->info("Success on '{$this->commandKey}'");
        }

        if ($eventName === self::EVENT_FAILURE) {
            // Log all failed calls to the client
            $this->logger->info("Failure on '{$this->commandKey}'");
        }

        if ($eventName === self::EVENT_SHORT_CIRCUITED) {
            $this->logger->info("Short circuited on '{$this->commandKey}'");
        }

        if ($eventName === self::EVENT_CIRCUIT_OPEN) {
            // Should be logged only once when circuit breaker is open
            $this->logger->critical("Circuit '{$this->commandKey}' is open.");
        }

        if ($eventName === self::EVENT_CIRCUIT_CLOSED) {
            // Should be logged only once when circuit breaker is closed
            $metrics = $this->getMetrics();
            $failures = $metrics->getRollingCount(MetricsCounter::SHORT_CIRCUITED);
            $message = "Circuit '{$this->commandKey}' is now closed. "
                . "There were {$failures} failures during the outage.";
            $this->logger->critical($message);
        }

        parent::processExecutionEvent($eventName);
    }
}

In current implementation there is no way to log the moment when circuit breaker is opened and closed. This should be a single execution event thrown only when circuit breaker is opened or closed. Other thing is that i can't get any metrics from my command class since getMetrics() method visibility is 'private'. Do you think this is something what could be useful to add ? Please let me know what you think.

Thank you in advance. Oleg G.

[lcf]

Hi @ogolubenco, you have full access to CommandMetricsFactory, ApcStateStorage and can decorate all the internal components, instrument them with logging or anything else.

That said, I don't mind making getMetrics protected and I don't mind adding two new types of events - although that would require some figuring out and possibly some refactoring, because currently working with the circuit is abstracted away and individual commands do not know whether the circuit was JUST open or has been open for a while. I would not go there, since there are other ways to get those events logged - e.b. by using your own implementation of ApcStateStorage.

[ogolubenco]

Hi @lcf, if i will use my own implementation of ApcStateStorage and will log open/close event in openCircuit() and closeCircuit() methods, how i can pass metrics to my ApcStateStorage implementation ?

Thank you

[lcf]

since you create the instance of CommandMetricsFactory yourself, you can pass it to the ApcStateStorage and then read stats for any command key.

[ogolubenco]

CommandMetricsFactory takes StateStorage as argument in constructor and passes it to MetricsCounter Looks like i will have to redo/extend all the logic to be able to get metrics in StateStorage which doesn't look like a good idea.

[lcf]

e.g. something like this

    class LoggingStateStorage extends ApcStateStorage{
        private $metricsFactory;
        public function __construct(CommandMetricsFactory $metricsFactory) {
            parent::__construct();
            $this->metricsFactory = $metricsFactory;
        }

        public function openCircuit($commandKey, $sleepingWindowInMilliseconds) {
            parent::openCircuit($commandKey, $sleepingWindowInMilliseconds);
            $metrics = $this->metricsFactory->get($commandKey, new Config([]));

            // here you can do the logging.
        }

        public function closeCircuit($commandKey) {
            // the same here...
        }
    }
    $stateStorage = new ApcStateStorage();
    $commandMetricsFactory = new CommandMetricsFactory($stateStorage);
    $circuitBreakerFactory = new CircuitBreakerFactory(new \LoggingStateStorage($commandMetricsFactory));

I have not tested, this is to illustrate the idea

[ogolubenco]

Hi, we can make it work this way. Please close this issue. Thank you

[lcf]

thank you for your feedback. the suggested refactoring is definitely something to consider in the future!