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

HystrixDashboard - threadpool does not display right TPS #1697

Open JagmohanSharma opened 6 years ago

JagmohanSharma commented 6 years ago

In order to fix issue with TPS in hystrix-dashboard, code changes were made previously in hystrixCommand.js https://github.com/Netflix/Hystrix/issues/1251, but as these changes were not done in hystrixThreadPool.js which causing a different value for TPS in threadpool section of dashboard.

 // the following will break when it becomes a compound string if the property is dynamically changed
        convertAvg(data, "propertyValue_metricsRollingStatisticalWindowInMilliseconds", false);

Above line of code was removed from below method in previous changes in hystrixCommand.js:

/**
     * Since the stream of data can be aggregated from multiple hosts in a tiered manner
     * the aggregation just sums everything together and provides us the denominator (reportingHosts)
     * so we must divide by it to get an average per instance value.
     *
     * We want to do this on any numerical values where we want per instance rather than cluster-wide sum.
     */
    function convertAllAvg(data) {
        convertAvg(data, "errorPercentage", true);
        convertAvg(data, "latencyExecute_mean", false);
        convertAvg(data, "latencyTotal_mean", false);

        // the following will break when it becomes a compound string if the property is dynamically changed
        convertAvg(data, "propertyValue_metricsRollingStatisticalWindowInMilliseconds", false);
    }

But same calculation is also being done in hystrixThreadPool.js as below:

function converAllAvg(data) {
        convertAvg(data, "propertyValue_queueSizeRejectionThreshold", false);

        // the following will break when it becomes a compound string if the property is dynamically changed
        convertAvg(data, "propertyValue_metricsRollingStatisticalWindowInMilliseconds", false);
    }

I think previously removed line(avg calculation for propertyValue_metricsRollingStatisticalWindowInMilliseconds) is also not required in hystrixThreadPool.js which causing wrong value for TPS in threadPool section of dashboard. I have updated code with this change and started a pull request for review. Please let me know if changes are good to go to fix this issue.

tjuchniewicz commented 6 years ago

Please see https://github.com/Netflix/Hystrix/issues/1251 In my opinion this feature is broken. Also calculation for commands is invalid. This is related to Turbine 1.0 and 2.0 incompatibility: https://github.com/Netflix/Turbine/issues/100

tjuchniewicz commented 6 years ago

Dashboard module moved to https://github.com/Netflix-Skunkworks/hystrix-dashboard