[Findit] Add settings for actions after culprits or suspects are identified.

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
index 64b986e43185e7b53617f3b881db7205e67449bb..a31e3c5f02bf281161314cd2f2d4577f4dc99898 100644
--- a/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
+++ b/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
@@ -14,11 +14,13 @@ from common.pipeline_wrapper import BasePipeline
 from common.rietveld import Rietveld
 from model import analysis_status as status
 from model.wf_culprit import WfCulprit
+from waterfall import waterfall_config
 
 
 @ndb.transactional
 def _ShouldSendNotification(
-    master_name, builder_name, build_number, repo_name, revision):
+    master_name, builder_name, build_number, repo_name, revision,
+    build_num_threshold, pass_time_limit):
   """Returns True if a notification for the culprit should be sent."""
   culprit = (WfCulprit.Get(repo_name, revision) or
              WfCulprit.Create(repo_name, revision))
@@ -26,11 +28,14 @@ def _ShouldSendNotification(
     return False
 
   culprit.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.
-  # TODO(stgao): move to config.
+  # Send notification only when:
+  # 1. It was not processed yet.
+  # 2. The culprit is for multiple failures in different builds to avoid false
+  #    positive due to flakiness.
+  # 3. It is not too late after the culprit was committed.
   should_send = not (culprit.cr_notification_processed or
-                     len(culprit.builds) < 2)
+                     len(culprit.builds) < build_num_threshold or
+                     pass_time_limit)
   if should_send:
     culprit.cr_notification_status = status.RUNNING
   culprit.put()
@@ -46,13 +51,9 @@ def _UpdateNotificationStatus(repo_name, revision, 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)
+def _SendNotificationForCulprit(repo_name, revision, code_review_url):
   sent = False
-  if change_log.code_review_url:
+  if code_review_url:
     # Occasionally, a commit was not uploaded for code-review.
     culprit = WfCulprit.Get(repo_name, revision)
     rietveld = Rietveld()
@@ -61,20 +62,47 @@ def _SendNotificationForCulprit(repo_name, revision):
     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)
+    sent = rietveld.PostMessage(code_review_url, message)
   else:
-    logging.error('Can not get code-review url for %s/%s', repo_name, revision)
+    logging.error('No code-review url for %s/%s', repo_name, revision)
 
   _UpdateNotificationStatus(repo_name, revision,
                             status.COMPLETED if sent else status.ERROR)
   return sent
 
 
+def _GetCulpritInfo(repo_name, revision):
+  """Returns commit time and code-review url of the given revision."""
+  # TODO(stgao): get repo url at runtime based on the given repo name.
+  # unused arg - pylint: disable=W0612,W0613
+  repo = GitRepository(
+      'https://chromium.googlesource.com/chromium/src.git', HttpClient())
+  change_log = repo.GetChangeLog(revision)
+  return change_log.committer_time, change_log.code_review_url
+
+
+def _PassNotificationTimeLimit(commit_time, latency_limit_seconds):

[lijeffrey, 2016/06/30 01:02:48]

nit: I would rename this _NotificationTimeLimitPassed(...) or something similar,
and the variable name 'pass_time_limit' (which without more context can sound
like it's meant to ignore a timeout) to 'time_limit_passed' or something to make
it more clear that this is indeed for timeout.

[stgao, 2016/06/30 17:35:33]

hm, this is actually not a timeout.
But the rename makes sense.

+  """Returns True if it is too late to send notification."""
+  latency_seconds = (datetime.utcnow() - commit_time).total_seconds()
+  return latency_seconds > latency_limit_seconds

[chanli, 2016/06/30 20:23:41]

Though I think this is just an implementation detail, but why do you want to use
seconds instead of minutes here? IMO, minutes are more reasonable here.

[stgao, 2016/06/30 22:36:57]

We could do minutes. Done.

+
+
 class SendNotificationForCulpritPipeline(BasePipeline):
 
   # Arguments number differs from overridden method - pylint: disable=W0221
   def run(self, master_name, builder_name, build_number, repo_name, revision):
+    action_settings = waterfall_config.GetActionSettings()
+    build_threshold = action_settings.get(
+        'cr_notification_build_threshold', 100000)
+    latency_limit_seconds = action_settings.get(
+        'cr_notification_latency_limit_seconds', 1)

[chanli, 2016/06/30 20:23:41]

are 100000 and 1 default values or some impossible values to prevent
notification to be sent?

[stgao, 2016/06/30 22:36:57]

Yes, added a comment on this.

+
+    commit_time, code_review_url = _GetCulpritInfo(repo_name, revision)
+    pass_time_limit = _PassNotificationTimeLimit(
+        commit_time, latency_limit_seconds)
+
     if not _ShouldSendNotification(
-      master_name, builder_name, build_number, repo_name, revision):
+      master_name, builder_name, build_number,
+      repo_name, revision, build_threshold, pass_time_limit):
       return False
-    return _SendNotificationForCulprit(repo_name, revision)
+    return _SendNotificationForCulprit(repo_name, revision, code_review_url)