First version of benchmark health report.

dashboard/dashboard/benchmark_quality_report.py

+# Copyright 2017 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Provides the web interface for a report on benchmark alert quality."""
+
+import datetime
+import json
+
+from dashboard import alerts
+from dashboard import list_tests
+from dashboard import update_test_suites
+from dashboard.common import request_handler
+from dashboard.common import utils
+from dashboard.models import anomaly
+
+
+class BenchmarkQualityReportHandler(request_handler.RequestHandler):
+  """Request handler for requests for group report page."""
+
+  def get(self):
+    """Renders the UI for the group report page."""
+    self.RenderStaticHtml('benchmark_quality_report.html')
+
+  def post(self):
+    """Returns data about the alerts for a benchmark.
+
+    Request parameters:
+      master: If specified, the master to list benchmarks for. If no arguments
+              are specified, benchmarks will be listed for chromium.perf.
+      benchmark: If specified, the name of the benchmark to gather details for.
+      num_days: With benchmark, number of days to get data for.
+      bug: If specified, the id of the bug to gather details for.
+
+    Outputs:
+      JSON for the /group_report page XHR request.
+    """
+    master = self.request.get('master')
+    benchmark = self.request.get('benchmark')
+    response_values = {}
+
+    if benchmark:
+      num_days = int(self.request.get('num_days'))
+      response_values = self._GetResponseValuesForBenchmark(
+          benchmark, num_days, master)
+    else:
+      response_values = self._GetResponseValuesForMaster(master)

[sullivan, 2017/02/17 16:30:13]

Current style of the dashboard is to have different responses based on
parameters. I think it is okay in this case, but I actually didn't implement any
bug/bisect stats yet because those don't map well onto this structure. Will file
a bug for that and we can discuss there?

[shatch, 2017/02/17 18:03:47]

sgtm

[eakuefner, 2017/02/17 19:11:05]

Caveat that I'm having a little bit of trouble parsing this, but what about
doing something like we do in /list_tests, where there's a |type| request
parameter and we expect different parameters based on that?

[sullivan, 2017/02/17 20:01:02]

I've been meaning to write up a bug/doc for discussion, I just wrote out
https://github.com/catapult-project/catapult/issues/3237

+
+    self.response.out.write(json.dumps(response_values))
+
+  def _GetResponseValuesForBenchmark(self, benchmark, num_days, master):
+    values = {}
+
+    # The cached test suite info contains info about monitoring and bots.
+    benchmarks = update_test_suites.FetchCachedTestSuites()
+    sheriff = self._GetSheriffForBenchmark(benchmark, master, benchmarks)
+    if sheriff:
+      query = anomaly.Anomaly.query(anomaly.Anomaly.sheriff == sheriff)
+      query = query.filter(anomaly.Anomaly.is_improvement == False)
+      query = query.filter(
+          anomaly.Anomaly.timestamp >
+          datetime.datetime.now() - datetime.timedelta(days=num_days))
+      query = query.order(-anomaly.Anomaly.timestamp)
+      anomalies = query.fetch()
+      anomalies = [a for a in anomalies if self._BenchmarkName(a) == benchmark]
+      values['monitored'] = True
+      values['alerts'] = alerts.AnomalyDicts(anomalies)
+    else:
+      values['monitored'] = False
+
+    values['bots'] = benchmarks[benchmark]['mas'][master].keys()
+    return values
+
+  def _GetSheriffForBenchmark(self, benchmark, master, benchmarks):

[sullivan, 2017/02/17 16:30:13]

This doesn't work if there are multiple sheriffs on a benchmark. Example that's
super confusing is the power sheriffing is the only thing listed for
system_health benchmark:
https://dev-sullivan-7c48f1f5-dot-chromeperf.appspot.com/benchmark_quality_re...

It shouldn't be too hard to extend it to grab multiple sheriffs, but I'd prefer
to do it in a follow up to make reviews easier.

[shatch, 2017/02/17 18:03:47]

Ah yeah was wondering why some of the pages returned fewer alerts than expected.
Add TODO just to note it?

[sullivan, 2017/02/17 18:28:42]

Done.

+    if not benchmarks[benchmark]['mon']:
+      return None
+    monitored_test_path = benchmarks[benchmark]['mon'][0]
+    pattern = '%s/*/%s/%s' % (master, benchmark, monitored_test_path)
+    monitored_tests = list_tests.GetTestsMatchingPattern(
+        pattern, list_entities=True)
+    return monitored_tests[0].sheriff
+
+  def _BenchmarkName(self, alert):
+    return utils.TestPath(alert.test).split('/')[2]
+
+  def _GetResponseValuesForMaster(self, master):
+    benchmarks = update_test_suites.FetchCachedTestSuites()

[sullivan, 2017/02/17 16:30:13]

We already have tons of information cached about the tests (what bots it has run
on, what is monitored, which things are marked "deprecated" after long periods
of no data). This endpoint re-uses that. Interested to hear how Simon and Ethan
feel about this after the problems with rampant re-use of the supposedly-cached
subtest dict.

[shatch, 2017/02/17 18:03:47]

Hadn't seen this, didn't know how bad it was. Don't really like this
subtestdict-like pattern of storing some massive monolithic object with all the
data in it, and retrieving it to just get at some subset. This thing seems to
need to loop querying 2000 objects at a time, does this twice in the overall
update. Poking through the datastore shows the externally visible version is
broken into 3 parts, which means it's somewhere between 2-3 Mb? And all you're
interested in here is a list of benchmark names for a master. Will file a bug to
investigate the different uses more and potentially break this down like we did
for subtest dict.

[sullivan, 2017/02/17 18:28:43]

Sounds good! This might turn into a good test case for how we could do things
differently, both in the current design and with a different data store.

[eakuefner, 2017/02/17 19:11:05]

I agree with both of you, that this is a place where we could probably do
something better. Caching is a nice performance optimization, but ideally we
would do something fast enough that it's not _required_ for functionality, and
ideally we would we would be able to enumerate situations where we need a list
of suites, and why. I'm as iffy about the ambiguity of what this dict is
actually for as I was about subtest dicts, specifically because of how
frequently this thing is used throughout add_point. So I'm in favor of an
analysis like Simon proposes. Maybe I'll take that on after I've closed out the
subtest dict refactor.

+    benchmarks = [b for b in benchmarks if master in benchmarks[b]['mas']]
+    return {'benchmarks': sorted(benchmarks)}