[Findit] Group failures by culprit and send notification to codereview.

appengine/findit/waterfall/send_notification_for_culprit_pipeline.py

Index: appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
diff --git a/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py b/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
new file mode 100644
index 0000000000000000000000000000000000000000..220c751430a8d97ec168677a1fdb159f3811330a
--- /dev/null
+++ b/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
@@ -0,0 +1,79 @@
+# Copyright 2016 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.
+
+import logging
+import textwrap
+
+from google.appengine.ext import ndb
+
+from common.git_repository import GitRepository
+from common.http_client_appengine import HttpClientAppengine as HttpClient
+from common.pipeline_wrapper import BasePipeline
+from common.rietveld import Rietveld
+from model import analysis_status as status
+from model.wf_culprit import WfCulprit
+
+
+@ndb.transactional
+def _ShouldSendNotification(
+    master_name, builder_name, build_number, repo_name, revision):
+  """Returns True if a notification for the culprit should be sent."""
+  culprit = WfCulprit.Get(repo_name, revision)

[lijeffrey, 2016/06/21 00:07:35]

nit: how about

culprit = WfCulprit.Get(repo_name, revision) or WfCulprit.Create(repo_name,
revision)?

[stgao, 2016/06/21 15:14:29]

Good idea! Done.

+  if not culprit:
+    culprit = WfCulprit.Create(repo_name, revision)
+  if [master_name, builder_name, build_number] in culprit.failed_builds:
+    return False
+
+  culprit.failed_builds.append([master_name, builder_name, build_number])
+  # Send notification when the culprit is for 2+ failures in two different
+  # builds to avoid false positive due to flakiness.

[chanli, 2016/06/21 17:50:31]

Do you have data to back up this idea? What percentage of false positives could
be filtered out by this?

[stgao, 2016/06/24 16:10:28]

This is from data in the InCorrect-Found. Only one case can't be caught by this
filter so far.

[chanli, 2016/06/24 23:42:38]

I'm not sure about it: I can see quite several cases where multiple builds
shared the same culprit but that culprit is incorrect.

For example:
401764: [(chromium.memory, Linux Chromium OS ASan LSan Tests (1), 13919),
(chromium.win, Win7 Tests (1), 54147), (chromium.win, Win7 (32) Tests, 6659),
...]

398149: [(chromium.chromiumos, ChromiumOS amd64-generic Compile, 19370),
(chromium.chromiumos, ChromiumOS daisy Compile, 21018), (chromium.chromiumos,
ChromiumOS x86-generic Compile, 20468)]

393771: [(chromium.chrome, Google Chrome Win, 7100), (chromium, Win, 43403)]

I would say we can go with this approach but we'd better wait for try jobs to be
more reliable

+  should_send = len(culprit.failed_builds) >= 2  # TODO(stgao): move to config.

[lijeffrey, 2016/06/21 00:07:34]

is it possible to merge these 2?

i.e.

should_send = (len(culprit.failed_builds >= 2 and
    culprit.notification_status not in (status.COMPLETED, status.RUNNING))

[stgao, 2016/06/21 15:14:29]

Done.

+  if culprit.notification_status in (status.COMPLETED, status.RUNNING):
+    should_send = False  # Already being processed by another pipeline.
+  if should_send:
+    culprit.notification_status = status.RUNNING
+  culprit.put()
+  return should_send
+
+
+@ndb.transactional
+def _UpdateNotificationStatus(repo_name, revision, new_status):
+  culprit = WfCulprit.Get(repo_name, revision)
+  culprit.notification_status = new_status
+  culprit.put()
+
+
+def _SendNotificationForCulprit(repo_name, revision):
+  # TODO(stgao): get repo url at runtime based on the given repo name.
+  repo = GitRepository(
+      'https://chromium.googlesource.com/chromium/src.git', HttpClient())
+  change_log = repo.GetChangeLog(revision)
+  sent = False
+  if change_log.code_review_url:
+    # Occasionally, a commit was not uploaded for code-review.
+    culprit = WfCulprit.Get(repo_name, revision)
+    rietveld = Rietveld()
+    message = textwrap.dedent("""
+    Findit Try-job identified this CL (revision %s) as the culprit for failures
+    in the build cycle(s) as shown on:
+    https://findit-for-me.appspot.com/waterfall/culprit?key=%s
+    """) % (revision, culprit.key().urlsafe())
+    sent = rietveld.PostMessage(change_log.code_review_url, message)
+  else:
+    logging.error('Can not get code-review url for %s/%s', repo_name, revision)
+
+  _UpdateNotificationStatus(repo_name, revision,
+                            status.COMPLETED if sent else status.ERROR)
+  return sent
+
+
+class SendNotificationForCulpritPipeline(BasePipeline):
+
+  # Arguments number differs from overridden method - pylint: disable=W0221
+  def run(self, master_name, builder_name, build_number, repo_name, revision):
+    if not _ShouldSendNotification(
+      master_name, builder_name, build_number, repo_name, revision):
+      return False
+    return _SendNotificationForCulprit(
+        master_name, builder_name, build_number, repo_name, revision)