[INDY-1468] move avgLatency computing to EMA algorithm #812

lampkin-diet · 2018-07-13T14:19:04Z

Signed-off-by: Andrew Nikitin andrew.nikitin@dsr-corporation.com

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

skhoroshavin · 2018-07-13T15:17:03Z

plenum/server/monitor.py

+                                          self._moving_average(curr_avg_lat,
+                                                               duration))
+
+    def _moving_average(self, p0, p1):


Renaming p0 to something like accum or old_accum and p1 back to next_val will make intentions more clear

skhoroshavin · 2018-07-13T15:20:37Z

plenum/server/monitor.py


    def add_request(self, ordered_ts):
        self.update_time(ordered_ts)
        self.reqs_in_window += 1

-    def _moving_average(self, next_val):
+    def add_duration(self, identifier, duration):
+        if identifier not in self.avg_latencies:


setdefault can be used here to avoid triple lookup

skhoroshavin · 2018-07-13T15:40:26Z

plenum/server/monitor.py


    def add_request(self, ordered_ts):
        self.update_time(ordered_ts)
        self.reqs_in_window += 1

-    def _moving_average(self, next_val):
+    def add_duration(self, identifier, duration):


Do we really need separate add_duration and add_request? Why not single add_request(id, timestamp, tto/time_to_order/latency)?

Also can we come up with some name for identifier so it can be more clear what it identifies?

skhoroshavin · 2018-07-13T15:46:47Z

plenum/server/monitor.py

+        total_reqs, curr_avg_lat = self.avg_latencies[identifier]
+        total_reqs += 1
+        self.avg_latencies[identifier] = (total_reqs,
+                                          self._moving_average(curr_avg_lat,


We are using moving average function to calculate average latency here, and it's using alpha which is based on throughput_min_cnt. Either "throughput" part should be removed from names of variables that control alpha, or separate set for latencies should be created.

Also here we're calculating moving average over latencies taking into account only their number, not their temporal placement. When we do the same with throughput we get measurements at well defined intervals, so we can really say that throughput is averaged over window with well defined duration. Yet this is not a case with latencies now, do we really intend that here?

skhoroshavin · 2018-07-13T15:50:41Z

plenum/server/monitor.py

-        pass
+    def get_avg_latency(self, identifier):
+        if identifier not in self.avg_latencies:
+            return .0


Why not None?

skhoroshavin · 2018-07-13T16:13:49Z

plenum/server/monitor.py

            return None
        self.update_time(request_time)
-        return self._moving_average(self.reqs_in_window / self.inner_window)
+        return self._moving_average(self.throughput, self.reqs_in_window / self.throughput_window_size)


After some thinking I came to conclusion that if we want to get smooth averages between window boundaries it's not that simple, and what's done here is not helping in any way. Imagine you had throughput of 1, then you measure it before you get another request and this gives you values less than 1, which is yet constant until you get a request, after which throughput jumps back to 1 and remains here until next window starts, at which point it jumps below 1 again. So, I believe right thing to do here is to either just return self.throughput or come up with better solution (which can turn out quite complicated). I would just stick with simple options for now.

ashcherbakov · 2018-07-16T08:07:13Z

plenum/test/monitoring/test_request_measurement_class.py

+    assert rm.avg_latencies['some_client_identifier'][1] != 0
+
+
+def test_avg_latency_accuracy(request_measurement):


Please add tests with different duration values

… max latency check Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

ashcherbakov · 2018-07-17T10:39:18Z

plenum/config.py

@@ -137,6 +137,11 @@
 LatencyWindowSize = 30
 LatencyGraphDuration = 240

+# This parameter defines minimal count of accumulated latencies for each client
+MIN_LATENCY_COUNT = 50


I think requiring 50 for each client is too much

ashcherbakov · 2018-07-17T10:39:30Z

plenum/config.py

+# This parameter defines minimal count of accumulated latencies for each client
+MIN_LATENCY_COUNT = 50
+# This parameter defines coefficient alpha, which represents the degree of weighting decrease.
+LATENCY_ALPHA = 0.2


Is it better than the Alpha based on MinCount?

…LATENCY_COUNT Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

skhoroshavin · 2018-07-18T08:41:03Z

plenum/server/monitor.py

        self.reqs_in_window = 0
        self.throughput = 0
-        self.inner_window = inner_window
-        self.min_cnt = min_cnt
+        self.throughput_window_size = throughput_window_size


Since class now is named ThroughputMeasurement probably there's no longer need in adding "thoughput" to some fields and parameter names. What about thoughput_window_size -> window_size, throughput_min_cnt -> min_window_count?

skhoroshavin · 2018-07-18T08:43:00Z

plenum/server/monitor.py


    def add_request(self, ordered_ts):
        self.update_time(ordered_ts)
        self.reqs_in_window += 1

-    def _moving_average(self, next_val):
+    def _accumulate(self, old_accum, next_val):


This is always called with old_accum= self.throughput, probably this parameter can be removed

skhoroshavin · 2018-07-18T08:50:17Z

plenum/server/monitor.py

-        self.inner_window = inner_window
-        self.min_cnt = min_cnt
+        self.throughput_window_size = throughput_window_size
+        self.throughput_min_cnt = throughput_min_cnt
        self.first_ts = time.perf_counter()


Timestamps are injected everywhere except here. This is both inconsistency and can lead to problems if someone decides to use some other timing function instead of time.perf_counter.

skhoroshavin · 2018-07-18T08:53:53Z

plenum/server/monitor.py

+    def add_duration(self, identifier, duration):
+        if identifier not in self.avg_latencies:
+            self.avg_latencies[identifier] = (0, .0)
+        total_reqs, curr_avg_lat = self.avg_latencies[identifier]


These 3 dict lookups can be replaced with just one:

total_reqs, curr_avg_lat = self.avg_latencies.get(identifier, (0, .0))

skhoroshavin · 2018-07-18T09:09:19Z

plenum/server/monitor.py

+                if avg_lat:
+                    if cid not in avgLatencies:
+                        avgLatencies[cid] = []
+                    avgLatencies[cid].append(avg_lat)


What about replacing triple lookup with

avgLatencies.setdefault(cid, []).append(avg_lat)

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

Andrew Nikitin added 2 commits July 13, 2018 17:15

[INDY-1468] move avgLatency computing to EMA algorithm

3f88d3a

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1468] flake8 fixes

987ed4d

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

skhoroshavin reviewed Jul 13, 2018

View reviewed changes

ashcherbakov reviewed Jul 16, 2018

View reviewed changes

[INDY-1468] Move latency measurement into different class and turnoff…

aa53de5

… max latency check Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

ashcherbakov reviewed Jul 17, 2018

View reviewed changes

[INDY-1468] decrease MIN_LATENCY_COUNT and calculate alpha using MIN_…

530d6c9

…LATENCY_COUNT Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

skhoroshavin reviewed Jul 18, 2018

View reviewed changes

[INDY-1468] some comment's improvements

88ec1df

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

skhoroshavin approved these changes Jul 19, 2018

View reviewed changes

ashcherbakov approved these changes Jul 19, 2018

View reviewed changes

ashcherbakov merged commit af19746 into hyperledger:master Jul 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INDY-1468] move avgLatency computing to EMA algorithm #812

[INDY-1468] move avgLatency computing to EMA algorithm #812

lampkin-diet commented Jul 13, 2018

skhoroshavin Jul 13, 2018

skhoroshavin Jul 13, 2018

skhoroshavin Jul 13, 2018

skhoroshavin Jul 13, 2018

skhoroshavin Jul 13, 2018

skhoroshavin Jul 13, 2018 •

edited

ashcherbakov Jul 16, 2018

ashcherbakov Jul 17, 2018 •

edited

ashcherbakov Jul 17, 2018

skhoroshavin Jul 18, 2018

skhoroshavin Jul 18, 2018

skhoroshavin Jul 18, 2018

skhoroshavin Jul 18, 2018 •

edited

skhoroshavin Jul 18, 2018 •

edited

		assert rm.avg_latencies['some_client_identifier'][1] != 0


		def test_avg_latency_accuracy(request_measurement):

[INDY-1468] move avgLatency computing to EMA algorithm #812

[INDY-1468] move avgLatency computing to EMA algorithm #812

Conversation

lampkin-diet commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skhoroshavin Jul 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashcherbakov Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skhoroshavin Jul 18, 2018 • edited

Choose a reason for hiding this comment

skhoroshavin Jul 18, 2018 • edited

Choose a reason for hiding this comment

skhoroshavin Jul 13, 2018 •

edited

ashcherbakov Jul 17, 2018 •

edited

skhoroshavin Jul 18, 2018 •

edited

skhoroshavin Jul 18, 2018 •

edited