Update DiskIo telemetry device to persist the counters #721

ebadyano · 2019-07-03T16:13:45Z

Ensure that DiskIo telemetry does not rely on Rally being a parent
process of Elasticsearch and persists the disk counters at the beginning
of a benchmark and can read it again afterwards.

Relates to #697

Ensure that DiskIo telemetry does not rely on Rally being a parent process of Elasticsearch and persists the disk counters at the beginning of a benchmark and can read it again afterwards. Relates to elastic#697

ebadyano · 2019-07-03T16:15:32Z

Accidentally clicked create PR before marking it a draft PR, so adding WIP to the title.

dliappis

Everything here looks rather sane to me, thanks! I haven't tested it with an external metric store yet but worked fine with the in-memory metrics store.

Update: I've also tested this PR (i.e. without #722) with an external metric store and seems to be alright.

I left some suggestions.

dliappis · 2019-07-04T15:09:56Z

esrally/mechanic/telemetry.py

+            read_bytes = 0
+            write_bytes = 0
+            io.ensure_dir(self.log_root)
+            log_file = "%s/%s-%s.io" % (self.log_root, self.car.safe_name, self.node.node_name)


This for example produces something like:

.rally/benchmarks/races/2019-07-04-15-05-34/rally-node-0/telemetry/defaults-rally-node-0.io

I don't think self.car.safe_name (in this case defaults) provides value in the filename, in which case you don't even need to pass car in the constructor. WDYT?

I agree, but I was trying to be consistent with what we do fo gcstats and jfr and other telemetry files. So not sure whether to go for simplicity or consistency. What's your preference?

Contrary to other telemetry devices this one is not meant to be inspected by the user. Instead this is a temporary state file that should also be removed when the benchmark ends (i.e. in on_benchmark_stop). Hence, I'd opt for simplicity here.

Also, we should maybe use a different name than log_file here? It's not exactly a log file.

Finally, I think we should use os.path.join instead of / when creating paths.

dliappis · 2019-07-04T15:12:49Z

esrally/mechanic/telemetry.py

                    self.logger.warning("Process I/O counters are not supported on this platform. Falling back to less accurate disk "
                                        "I/O counters.")
                except RuntimeError:
                    self.logger.exception("Could not determine I/O stats at benchmark start.")
+            with open(log_file, "wt", encoding="utf-8") as f:
+                diskio_str = "%d %d" % (read_bytes, write_bytes)


Have you considered writing to a dict and dumping it as json instead? What you have here works perfectly, but I wonder if standardizing to json for all structures we have in Rally makes sense.

So here we'd have {"read_bytes": read_bytes, "write_bytes": write_bytes} and then on the next line it could be json.dump(diskio_str, f))

I like your json idea, I will change it to use that. Thanks!

dliappis · 2019-07-04T15:14:01Z

esrally/mechanic/telemetry.py

+            log_file = "%s/%s-%s.io" % (self.log_root, self.car.safe_name, self.node.node_name)
+            io_str = ""
+            with open(log_file, "rt", encoding="utf-8") as f:
+                io_str = f.read()


See my earlier suggestion whether we could json.load instead and use it as a dictionary without the need to split.

dliappis · 2019-07-04T15:15:27Z

esrally/mechanic/telemetry.py

-                    write_bytes = process_end.write_bytes - self.process_start.write_bytes
-                elif self.disk_start:
+                if process_end:
+                    read_bytes = process_end.read_bytes - int(io_bytes[0])


The json approach, as a benefit, can ensure type safety in a type unsafe language and you can get rid of the int conversions.

dliappis · 2019-07-04T15:28:50Z

esrally/mechanic/launcher.py

@@ -295,7 +295,7 @@ def _start_node(self, node_configuration, node_count_on_host):
        enabled_devices = self.cfg.opts("mechanic", "telemetry.devices")
        telemetry_params = self.cfg.opts("mechanic", "telemetry.params")
        node_telemetry = [
-            telemetry.DiskIo(self.metrics_store, node_count_on_host),
+            telemetry.DiskIo(self.metrics_store, node_count_on_host, node_telemetry_dir, car, node_name),


See my question below whether car is actually needed.

ebadyano · 2019-07-04T18:34:10Z

@dliappis I wonder if I do need to do something about self.metrics_store as well. It's initialized in the launcher, but by the time stop/start is executed we don't have it anymore..

dliappis · 2019-07-05T11:59:47Z

@dliappis I wonder if I do need to do something about self.metrics_store as well. It's initialized in the launcher, but by the time stop/start is executed we don't have it anymore..

@ebadyano this needs to be looked together with #722; as per #697 (comment) the race-id will be provided to the start/stop commands and each should still initialize the metric stores used for recoding telemetry and other metric records so in theory if all this initialization is done in #722 the code here should work as is.

danielmitterdorfer

I left a few suggestions.

danielmitterdorfer · 2019-07-12T07:48:32Z

esrally/mechanic/telemetry.py

+            write_bytes = 0
+            io.ensure_dir(self.log_root)
+            log_file = "%s/%s-%s.io" % (self.log_root, self.car.safe_name, self.node.node_name)
+            console.info("%s: Writing start I/O stats to [%s]" % (self.human_name, log_file), logger=self.logger)


As this is only a temporary file that is necessary due to the way the start and stop subcommands will work, we should not inform the user about it.

danielmitterdorfer · 2019-07-12T07:49:56Z

esrally/mechanic/telemetry.py

                    self.logger.warning("Process I/O counters are not supported on this platform. Falling back to less accurate disk "
                                        "I/O counters.")
                except RuntimeError:
                    self.logger.exception("Could not determine I/O stats at benchmark start.")
+            diskio_str = {"read_bytes": read_bytes, "write_bytes": write_bytes}


Should we avoid the _str suffix? Also, I think we could just inline the dict declaration and get rid of the variable completely.

danielmitterdorfer · 2019-07-12T07:51:49Z

esrally/mechanic/telemetry.py

+            process_end = sysstats.process_io_counters(self.process)
+            disk_end = sysstats.disk_io_counters()
+            io.ensure_dir(self.log_root)
+            log_file = "%s/%s-%s.io" % (self.log_root, self.car.safe_name, self.node.node_name)


Can we use os.path.join instead of / here?

danielmitterdorfer · 2019-07-12T07:52:33Z

esrally/mechanic/telemetry.py

+            disk_end = sysstats.disk_io_counters()
+            io.ensure_dir(self.log_root)
+            log_file = "%s/%s-%s.io" % (self.log_root, self.car.safe_name, self.node.node_name)
+            io_str = ""


danielmitterdorfer · 2019-07-12T07:53:49Z

esrally/mechanic/telemetry.py

-                    write_bytes = process_end.write_bytes - self.process_start.write_bytes
-                elif self.disk_start:
+                if process_end:
+                    read_bytes = process_end.read_bytes - io_bytes['read_bytes']


We should use double quotes for string constants by default (there are cases where it makes sense to use single quotes, e.g. when there are a lot of double quotes used in the string constant, it is easier to use single quotes but here this is not the case).

danielmitterdorfer · 2019-07-12T07:56:05Z

esrally/mechanic/telemetry.py

@@ -776,17 +777,21 @@ def _store_merge_times(self, merge_times):


 class DiskIo(InternalTelemetryDevice):
+    human_name = "Disk IO"


when we get rid of the console message (see my comment below), this will be unused and we can remove it as well.

esrally/mechanic/telemetry.py

ebadyano · 2019-07-16T13:32:54Z

@danielmitterdorfer Thank you for the review, I addressed your comments. Could you please take another look? Thanks!

With this commit we remove support in Rally to benchmark Elasticsearch 1.x. Relates elastic#715 Relates elastic#716

With this commit we add the JVM flag `ExitOnOutOfMemoryError` unconditionally when Elasticsearch is configured by Rally. Previously we had to check whether the JVM in use supports this flag because we supported Elasticsearch 1.x which can be run with Java 1.7. As the JVM flag has only been introduced with Java 8, we had a check in place. Now that we have dropped support for Elasticsearch 1.x (in elastic#716), we can safely assume that the JVM supports this flag and unconditionally set it. Relates elastic#715 Relates elastic#723

To check the complete list of exposed track parameters in an integration test we rely on parameters in rally-tracks which can change over time[1]. Be more lenient by just checking the unused track parameters. Relates elastic#726 [1] elastic/rally-tracks@4080dc9

With this commit we allow (again) to set the distribution version as a command line parameter even in cases where it does not seem to be useful initially, like the `benchmark-only` pipeline. However, if Rally fails to determine the cluster version (for whatever reason) it can be useful to be able to override this on the command line. Similarly, it can be useful to override this when benchmarking a source build that is *not* based on master but rather of an older branch. In these cases, Rally would still take the right decisions with some guidance by the user. Relates elastic#728

With this commit we remove support for Elasticsearch 1.x when retrieving cluster metadata. This is a leftover from elastic#716 Relates elastic#716 Relates elastic#729

danielmitterdorfer

I have a couple of comments.

danielmitterdorfer · 2019-07-16T14:27:24Z