Allow compare to continue for missing disk usage stats

[inqueue]

This commit fixes an issue introduced by the one with the beaker when running esrally compare where disk usage stats are not present for prior benchmarks.

$ esrally compare --baseline=1ad6f592-14c7-4ffd-8c93-4eef6555eb9d --contender=bfdfe50a-d62f-45df-ac9a-6a1a5295d45f

    ____        ____
   / __ \____ _/ / /_  __
  / /_/ / __ `/ / / / / /
 / _, _/ /_/ / / / /_/ /
/_/ |_|\__,_/_/_/\__, /
                /____/


Comparing baseline
  Race ID: 1ad6f592-14c7-4ffd-8c93-4eef6555eb9d
-  Race timestamp: 2021-08-09 20:00:56
  Challenge: logging-querying
  Car: external
  User tags: env-id=4d577a92-4cf4-4da2-b1dd-948efb4a6d15

with contender
  Race ID: bfdfe50a-d62f-45df-ac9a-6a1a5295d45f
  Race timestamp: 2022-05-04 19:09:41
  Challenge: logging-querying
  Car: external
  User tags: env-id=18de9c61-7975-4aa9-800c-d2a984299268, setup=esbench

------------------------------------------------------
    _______             __   _____
   / ____(_)___  ____ _/ /  / ___/_________  ________
  / /_  / / __ \/ __ `/ /   \__ \/ ___/ __ \/ ___/ _ \
 / __/ / / / / / /_/ / /   ___/ / /__/ /_/ / /  /  __/
/_/   /_/_/ /_/\__,_/_/   /____/\___/\____/_/   \___/
------------------------------------------------------

[ERROR] Cannot compare. 'NoneType' object is not iterable.

rally.log

2022-05-06 21:26:32,719 -not-actor-/PID:70929 esrally.rally ERROR A fatal error occurred while running subcommand [compare].
Traceback (most recent call last):
  File "/Users/jbryan/dev/src/rally/esrally/rally.py", line 910, in dispatch_sub_command
    reporter.compare(cfg, args.baseline, args.contender)
  File "/Users/jbryan/dev/src/rally/esrally/reporter.py", line 49, in compare
    ComparisonReporter(cfg).report(race_store.find_by_race_id(baseline_id), race_store.find_by_race_id(contender_id))
  File "/Users/jbryan/dev/src/rally/esrally/reporter.py", line 397, in report
    metric_table_plain = self._metrics_table(baseline_stats, contender_stats, plain=True)
  File "/Users/jbryan/dev/src/rally/esrally/reporter.py", line 415, in _metrics_table
    metrics_table.extend(self._report_disk_usage_stats_per_field(baseline_stats, contender_stats))
  File "/Users/jbryan/dev/src/rally/esrally/reporter.py", line 652, in _report_disk_usage_stats_per_field
    for index, total, field in total_disk_usage_per_field(baseline_stats):
  File "/Users/jbryan/dev/src/rally/esrally/reporter.py", line 112, in total_disk_usage_per_field
    for field_stat in stats.disk_usage_total:
TypeError: 'NoneType' object is not iterable

[DJRickyB]

should we only perform this work at all if both have disk_usage_total? so instead of these changes do (in line 415)

if baseline_stats.disk_usage_total and contender_stats.disk_usage_total:
    metrics_table.extend(self._report_disk_usage_stats_per_field(baseline_stats, contender_stats))

[nik9000]

That makes sense. Can we issue a warning about it? Something on the console?

[inqueue]

should we only perform this work at all if both have disk_usage_total? so instead of these changes do (in line 415)

I'm good with this since there can be no comparison if either element contains non-existent objects. I'll give it a try to ensure it does what we want.

[inqueue]

That makes sense. Can we issue a warning about it? Something on the console?

I was ready to add a warning until I thought it would add noise when running comparisons against metrics generated with older Rally versions and disk_usage_total=[] for later versions. I'm interested in your thoughts.

What do you think instead about setting <x>.disk_usage_total=[] when <x>.disk_usage_total=None? This way we'll show disk usage metrics if they exist for one of the subjects, and not leave anything out.

[nik9000]

Either way is fine by me. I think if we are going to skip them all we should print a warning if one is empty. But I don't know the exact mechanisms here. Sorry!

…

On Wed, May 11, 2022, 6:07 PM Jason Bryan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In esrally/reporter.py <#1482 (comment)>: > @@ -648,22 +648,23 @@ def _report_ingest_pipeline_failed(self, baseline_stats, contender_stats): def _report_disk_usage_stats_per_field(self, baseline_stats, contender_stats): best = {} - for index, total, field in total_disk_usage_per_field(baseline_stats): - best.setdefault(index, {})[field] = total - for index, total, field in total_disk_usage_per_field(contender_stats): - for_idx = best.setdefault(index, {}) - prev = for_idx.get(field, 0) - if prev < total: - for_idx[field] = total + if baseline_stats.disk_usage_total: That makes sense. Can we issue a warning about it? Something on the console? I was ready to add a warning until I thought it would add noise when running comparisons against metrics generated with older Rally versions and disk_usage_total=[] for later versions. I'm interested in your thoughts. What do you think instead about setting <x>.disk_usage_total=[] when <x>.disk_usage_total=None? This way we'll show disk usage metrics if they exist for one of the subjects, and not leave anything out. — Reply to this email directly, view it on GitHub <#1482 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABUXIVFOO3UBROZYYHFJDLVJQVQPANCNFSM5VJLSO2Q> . You are receiving this because your review was requested.Message ID: ***@***.***>

[DJRickyB]

I don't think a warning is appropriate. currently when we run compare on two un-alike (for whatever reason) races, it just quietly filters out rows that are not shared between the two races. This issue is just a bugfix for a missing field from older indices, I think the overall behavior of compare could be discussed in a different issue

[DJRickyB]

LGTM