[Findit] Add findit_for_client to do analysis based on client_id

appengine/findit/crash/findit_for_client.py

Index: appengine/findit/crash/findit_for_client.py
diff --git a/appengine/findit/crash/findit_for_client.py b/appengine/findit/crash/findit_for_client.py
new file mode 100644
index 0000000000000000000000000000000000000000..e52a5cef271e461d0997a707f6b6e5801f508caa
--- /dev/null
+++ b/appengine/findit/crash/findit_for_client.py
@@ -0,0 +1,162 @@
+# 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.
+
+"""This module is interfacas between clients-specific code and core.
+
+Note, fracas and cracas are almost identical and fracas is an intermidiate
+state while transfering to cracas, so they can be handled in the same code path
+and can be refered to as chromecrash."""

[lijeffrey, 2016/09/16 16:28:56]

nit: referred

[Sharu Jiang, 2016/09/19 21:36:00]

Done.

+
+import copy
+import json
+import logging
+
+from google.appengine.ext import ndb
+
+from common import appengine_util
+from common import time_util
+from crash import findit_for_chromecrash
+from crash.type_enums import CrashClient
+from model import analysis_status
+from model.crash.crash_config import CrashConfig
+from model.crash.fracas_crash_analysis import FracasCrashAnalysis
+
+_FINDIT_FRACAS_FEEDBACK_URL_TEMPLATE = '%s/crash/fracas-result-feedback?key=%s'
+
+
+def CheckPolicyForClient(crash_identifiers, chrome_version, signature,
+                         client_id, platform, stack_trace, customized_data):
+  """Checks if the args pass the client policy or not, update the parameters

[lijeffrey, 2016/09/16 16:28:56]

nit: I'm not sure this docstring is formatted correctly. Is it possible to get
the description on 1 line, then explain each parameter:

"""Checks if args pass client policy and updates parameters.

Args:
  crash_identifiers: ...
  ...
"""

[Sharu Jiang, 2016/09/19 21:36:00]

Done.

+  if needed."""
+  crash_config = CrashConfig.Get()
+
+  # Cracas and Fracas share the sampe policy.
+  if client_id == CrashClient.FRACAS or client_id == CrashClient.CRACAS:
+    if platform not in crash_config.fracas.get(

[Martin Barbella, 2016/09/17 23:20:35]

Same concerns here that were mentioned in the other files.

Any misspelling will lead to hard to detect errors, especially with the defaults
used. It may be safer to require that these are set, and have a helper that
validates that the config looks good or fails loudly at the start if it doesn't.

[Sharu Jiang, 2016/09/19 21:36:00]

Right, this is in the TODO plans, will do this in another cl.

+        'supported_platform_list_by_channel', {}).get(
+            customized_data.get('channel'), []):

[lijeffrey, 2016/09/16 16:28:56]

split out customized_data.get('channel') since it's being used in the logging
too, and makes this line a bit more readable

[Sharu Jiang, 2016/09/19 21:35:59]

Done.

+      # Bail out if either the channel or platform is not supported yet.
+      logging.info('Ananlysis of channel %s, platform %s is not supported. '
+                   'No analysis is scheduled for %s',
+                   customized_data.get('channel'), platform,
+                   repr(crash_identifiers))
+      return False, None
+
+    # TODO(katesonia): Remove the default value after adding validity check to
+    # config.
+    for blacklist_marker in crash_config.fracas.get(
+        'signature_blacklist_markers', []):
+      if blacklist_marker in signature:
+        logging.info('%s signature is not supported. '
+                     'No analysis is scheduled for %s', blacklist_marker,
+                     repr(crash_identifiers))
+        return False, None
+
+    # TODO(katesonia): Remove the default value after adding validity check to
+    # config.
+    platform_rename = crash_config.fracas.get('platform_rename', {})
+    platform = platform_rename.get(platform, platform)
+
+  # TODO(katesonia): Add clusterfuzz policy check.

[lijeffrey, 2016/09/16 16:28:55]

nit: I would move this todo to inside the if statement like you have with
GetAnalysisForClient

[Sharu Jiang, 2016/09/19 21:36:00]

Done.

+  elif client_id == CrashClient.CLUSTERFUZZ:  # pragma: no cover.
+    pass
+
+  return True, (crash_identifiers, chrome_version, signature, client_id,
+                platform, stack_trace, customized_data)
+
+
+def GetAnalysisForClient(crash_identifiers, client_id):
+  analysis = None
+  if client_id == CrashClient.FRACAS:
+    analysis = FracasCrashAnalysis.Get(crash_identifiers)
+  elif client_id == CrashClient.CRACAS:  # pragma: no cover.
+    # TODO(katesonia): Add CracasCrashAnalysis model.
+    pass
+  elif client_id == CrashClient.CLUSTERFUZZ:  # pragma: no cover.
+    # TODO(katesonia): Add ClusterfuzzCrashAnalysis model.
+    pass
+
+  return analysis
+
+
+def ResetAnalysis(analysis, crash_identifiers, chrome_version, signature,
+                  client_id, platform, stack_trace, customized_data):
+  """Resets analysis and returns if analysis get successfully reset or not."""

[lijeffrey, 2016/09/16 16:28:56]

nit: """... and returns whether analysis was successfully reset."""

[Sharu Jiang, 2016/09/19 21:35:59]

Done.

+  if not analysis:

[Martin Barbella, 2016/09/17 23:20:35]

What's the intention of this? I don't fully understand what this function is
intended to do. A bit more elaboration in the docstring would also be nice.

[Sharu Jiang, 2016/09/19 21:35:59]

If it's the first time to analyze this crash, we need to create an analysis
entity, if it's a rerun, we only need to reset those information.

I move the create new analysis out of this function and rename the function to
SetAnalysis, I think this way is more clear :)

+    # A new analysis is needed if there is no analysis.
+    if client_id == CrashClient.FRACAS:
+      analysis = FracasCrashAnalysis.Create(crash_identifiers)
+    elif client_id == CrashClient.CRACAS:  # pragma: no cover.
+      # TODO(katesonia): define CracasCrashAnalysis.
+      return False
+    elif client_id == CrashClient.CLUSTERFUZZ: # pragma: no cover.
+      # TODO(katesonia): define ClusterfuzzCrashAnalysis.
+      return False
+    else:
+      # The client id is not supported.
+      return False
+
+  analysis.Reset()
+
+  # Set common properties.
+  analysis.crashed_version = chrome_version
+  analysis.stack_trace = stack_trace
+  analysis.signature = signature
+  analysis.platform = platform
+  analysis.client_id = client_id
+
+  if client_id == CrashClient.FRACAS or client_id == CrashClient.CRACAS:
+    # Set customized properties.
+    analysis.historical_metadata = customized_data.get('historical_metadata')
+    analysis.channel = customized_data.get('channel')
+  elif client_id == CrashClient.CLUSTERFUZZ:  # pragma: no cover.
+    # TODO(katesonia): Set up clusterfuzz customized data.
+    pass
+
+  # Set analysis progress properties.
+  analysis.status = analysis_status.PENDING
+  analysis.requested_time = time_util.GetUTCNow()
+
+  analysis.put()
+  return True
+
+
+def GetPublishResultFromAnalysis(analysis, crash_identifiers, client_id):
+  """Gets result to be published to client from datastore analysis."""
+  analysis_result = copy.deepcopy(analysis.result)
+
+  if (analysis.client_id == CrashClient.FRACAS or

[Martin Barbella, 2016/09/17 23:20:35]

Seems like this pattern has come up a few times. May be worth having a helper
for this. I don't feel strongly about it, though.

[Sharu Jiang, 2016/09/19 21:35:59]

I think we can leave it as it is, since it is very self-explaining :)

+      analysis.client_id == CrashClient.CRACAS):
+    analysis_result['feedback_url'] = _FINDIT_FRACAS_FEEDBACK_URL_TEMPLATE % (
+        appengine_util.GetDefaultVersionHostname(), analysis.key.urlsafe())
+    if analysis_result['found']:
+      for cl in analysis_result['suspected_cls']:
+        cl['confidence'] = round(cl['confidence'], 2)
+        cl.pop('reason', None)
+  elif client_id == CrashClient.CLUSTERFUZZ:  # pragma: no cover.
+    # TODO(katesonia): Post process clusterfuzz analysis result if needed.
+    pass
+
+  return {
+      'crash_identifiers': crash_identifiers,
+      'client_id': analysis.client_id,
+      'result': analysis_result,
+  }
+
+
+def FindCulprit(analysis):
+  result = {'found': False}
+  tags = {'found_suspects': False,
+          'has_regression_range': False}
+
+  if (analysis.client_id == CrashClient.FRACAS or
+      analysis.client_id == CrashClient.CRACAS):
+    result, tags = findit_for_chromecrash.FindCulpritForChromeCrash(
+        analysis.signature, analysis.platform, analysis.stack_trace,
+        analysis.crashed_version, analysis.historical_metadata)
+  elif analysis.client_id == CrashClient.CLUSTERFUZZ:  # pragma: no cover.
+    # TODO(katesonia): Implement findit_for_clusterfuzz.
+    pass
+
+  return result, tags