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

appengine/findit/waterfall/identify_try_job_culprit_pipeline.py

Index: appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
diff --git a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
index 56aed34d4b303d4a1e1998eb2203781d65614329..0a7642ef567cd1718e1276af959ad99616fd9863 100644
--- a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
+++ b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 
 from collections import defaultdict
+import logging
 
 from google.appengine.ext import ndb
 
@@ -15,6 +16,8 @@ from model.wf_analysis import WfAnalysis
 from model.wf_try_job import WfTryJob
 from model.wf_try_job_data import WfTryJobData
 from waterfall.try_job_type import TryJobType
+from waterfall.send_notification_for_culprit_pipeline import \

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

nit: I think gpylint doesn't like multi-line lines using \

is it possible to do something like:

from waterfall.send_notification_for_culprit_pipeline import (
    SendNotificationForCulpritPipeline)?

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

Done.

+    SendNotificationForCulpritPipeline
 
 
 GIT_REPO = GitRepository(
@@ -97,6 +100,18 @@ def _GetSuspectedCLs(analysis, result):
   return suspected_cls
 
 
+def _NotifyCulprits(master_name, builder_name, build_number, culprits):
+  """Sends notifications to the identified culprits."""
+  try:
+    for culprit in (culprits or {}).itervalues():
+      pipeline = SendNotificationForCulpritPipeline(
+          master_name, builder_name, build_number,
+          culprit['repo_name'], culprit['revision'])
+      pipeline.start()
+  except Exception:
+    logging.exception('Failed to notify culprits.')
+
+
 class IdentifyTryJobCulpritPipeline(BasePipeline):
   """A pipeline to identify culprit CL info based on try job compile results."""
 
@@ -326,4 +341,5 @@ class IdentifyTryJobCulpritPipeline(BasePipeline):
     # Add try-job results to WfAnalysis.
     UpdateWfAnalysisWithTryJobResult()
 
+    _NotifyCulprits(master_name, builder_name, build_number, culprits)

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

So only culprits found by try jobs will be included, right? And that's expected?

If so, when do you want to enable this change in prod? I mean, should we wait
for the accuracy rate for try jobs to get higher?

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

That's what I thought. Are you proposing heuristic results should be notified
too?
As in the other pipeline, we only notify if the same culprit is identified for
2+ different builders. I think that's good enough to avoid the false positive
caused by flaky compile.
Did you find some cases that it won't work?

I was supposed to add a flag to disable the notification.
But it will be in a separate CL.

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

Such as the case where the culprit revision is skipped by analyze? 

     return result.get('culprit') if result else None