Netflix / Hystrix

Hystrix is a latency and fault tolerance library designed to isolate points of access to remote systems, services and 3rd party libraries, stop cascading failure and enable resilience in complex distributed systems where failure is inevitable.
24.07k stars 4.7k forks source link

Command names must be unique for the metrics to be correctly reported #1888

Open maciejp opened 5 years ago

maciejp commented 5 years ago

Using Hystrix 1.5.13, suppose you have a command with following parameters:

HystrixCommandGroupKey = Police HystrixCommandKey = CreateEmergency

When it gets executed, the following metrics will be reported

name=Police.CreateEmergency.countBadRequests, value=0
name=Police.CreateEmergency.countCollapsedRequests, value=0
[...]

I would expect that adding a second command with a different group key, but identical command key: HystrixCommandGroupKey = FireBrigade HystrixCommandKey = CreateEmergency

will result in having additional metrics:

name=Police.CreateEmergency.countBadRequests, value=0
name=Police.CreateEmergency.countCollapsedRequests, value=0
name=FireBrigade.CreateEmergency.countBadRequests, value=0
name=FireBrigade.CreateEmergency.countCollapsedRequests, value=0
[...]

But it is not the case. What happens is that metrics of the later executed method are not published.

Turns out that because of this part command keys must be unique to be correctly published.

    /* package */ HystrixMetricsPublisherCommand getPublisherForCommand(HystrixCommandKey commandKey, HystrixCommandGroupKey commandOwner, HystrixCommandMetrics metrics, HystrixCircuitBreaker circuitBreaker, HystrixCommandProperties properties) {
        // attempt to retrieve from cache first
        HystrixMetricsPublisherCommand publisher = commandPublishers.get(commandKey.name());
        if (publisher != null) {
            return publisher;
        } else {
            synchronized (this) {
                HystrixMetricsPublisherCommand existingPublisher = commandPublishers.get(commandKey.name());
                if (existingPublisher != null) {
                    return existingPublisher;
                } else {
                    HystrixMetricsPublisherCommand newPublisher = HystrixPlugins.getInstance().getMetricsPublisher().getMetricsPublisherForCommand(commandKey, commandOwner, metrics, circuitBreaker, properties);
                    commandPublishers.putIfAbsent(commandKey.name(), newPublisher);
                    newPublisher.initialize();
                    return newPublisher;
                }
            }
        }
    }

The code above gets executed on HystrixMetricsPublisherFactory SINGLETON when a command is created (in AbstractCommand's constructor) which means that if there are two commands with the same name, only the first command to be executed gets its HystrixMetricsPublisherCommand added to the map. Hence only metrics about the first executed command will be reported.

This behavior is confusing, I would expect both the HystrixCommandGroupKey and HystrixCommandKey to be considered as uniquely defining a command. Is using only the HystrixCommandKey by design?

Cheers

dnsbtchr commented 4 years ago

Hi, are there any plans to fix this by merging the requested PR soon?