[Findit] Add findit_for_client to do analysis based on client_id

[Findit] Add findit_for_client to do analysis based on client_id

Add findit_for_client on top of findit_for_fracas and findit_for_clusterfuzz(to be implemented) to do client-specific checking and call different module to do analysis based on client_id.

Doc: https://docs.google.com/a/google.com/document/d/1XkGjz7O2WNoGz2_x2JpbaCFZKsKqYpeLy9LiwfgKsZA/edit?usp=sharing

BUG=645203
Committed: https://chromium.googlesource.com/infra/infra/+/bdee0f60631fbffc47f4ca7c58b9b1241da8af86

[Sharu Jiang, 2016-09-08 20:05:55]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2016-09-08 20:06:02]

Patchset #1 (id:20001) has been deleted

[Sharu Jiang, 2016-09-08 20:11:37]

Patchset #1 (id:40001) has been deleted

[Sharu Jiang, 2016-09-08 20:28:26]

Description was changed from

==========
[Findit] Move those client specific check into ScheduleNewAnalysisForCrash

BUG=
==========

to

==========
[Findit] Add findit_for_client to do analysis based on client_id

Add findit_for_client to do client-specific checking and call different module
to do analysis based on client_id.

BUG=645203
==========

[Sharu Jiang, 2016-09-08 20:28:27]

katesonia@chromium.org changed reviewers:
+ chanli@chromium.org, mbarbella@google.com, mmandlis@chromium.org,
stgao@chromium.org

[Sharu Jiang, 2016-09-08 20:30:58]

Description was changed from

==========
[Findit] Add findit_for_client to do analysis based on client_id

Add findit_for_client to do client-specific checking and call different module
to do analysis based on client_id.

BUG=645203
==========

to

==========
[Findit] Add findit_for_client to do analysis based on client_id

Add findit_for_client on top of findit_for_fracas and findit_for_clusterfuzz(to
be implemented) to do client-specific checking and call different module to do
analysis based on client_id.

BUG=645203
==========

[Sharu Jiang, 2016-09-08 20:31:05]

Patchset #1 (id:60001) has been deleted

[Sharu Jiang, 2016-09-08 20:31:18]

PTAL :)

[Sharu Jiang, 2016-09-08 20:38:13]

katesonia@chromium.org changed reviewers:
+ wrengr@chromium.org
- chanli@chromium.org

[Sharu Jiang, 2016-09-08 20:39:37]

Description was changed from

==========
[Findit] Add findit_for_client to do analysis based on client_id

Add findit_for_client on top of findit_for_fracas and findit_for_clusterfuzz(to
be implemented) to do client-specific checking and call different module to do
analysis based on client_id.

BUG=645203
==========

to

==========
[Findit] Add findit_for_client to do analysis based on client_id

Add findit_for_client on top of findit_for_fracas and findit_for_clusterfuzz(to
be implemented) to do client-specific checking and call different module to do
analysis based on client_id.

Doc:
https://docs.google.com/a/google.com/document/d/1XkGjz7O2WNoGz2_x2JpbaCFZKsKq...

BUG=645203
==========

[stgao, 2016-09-13 16:37:31]

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:92: logging.info('Published analysis
result for %s', repr(crash_identifiers))
include client_id

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:103: def _NeedsNewAnalysis(
If we remove the transaction annotation, there is a risk that two analyses might
run at the same time for the same crash.

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:106: pass_policy, args =
findit_for_client.CheckPolicyForClient(
`args` here is too general. Change to a name that is self-explain.

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
File appengine/findit/crash/findit_for_client.py (right):

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:42: for blacklist_marker in
crash_config.fracas.get(
At top of this file, should we add a docstring that explains crash from Cracas
and Fracas could be handled in the same code path?

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:57: pass
Will this trigger a pipeline to run the analysis?

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:87: pass
Will this result int error later in the code below?

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:123:
app_identity.get_default_version_hostname(), analysis.key.urlsafe())
Should we move the hostname part to common/appengine_util.py?

[Sharu Jiang, 2016-09-14 20:38:38]

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:92: logging.info('Published analysis
result for %s', repr(crash_identifiers))
On 2016/09/13 16:37:31, stgao (slow) wrote:
> include client_id

Done.

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:103: def _NeedsNewAnalysis(
On 2016/09/13 16:37:31, stgao (slow) wrote:
> If we remove the transaction annotation, there is a risk that two analyses
might
> run at the same time for the same crash.

Done.

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:106: pass_policy, args =
findit_for_client.CheckPolicyForClient(
On 2016/09/13 16:37:31, stgao (slow) wrote:
> `args` here is too general. Change to a name that is self-explain.

How about updated_analysis_args or tuned_analysis_args?

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
File appengine/findit/crash/findit_for_client.py (right):

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:42: for blacklist_marker in
crash_config.fracas.get(
On 2016/09/13 16:37:31, stgao (slow) wrote:
> At top of this file, should we add a docstring that explains crash from Cracas
> and Fracas could be handled in the same code path?

Done.

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:57: pass
On 2016/09/13 16:37:31, stgao (slow) wrote:
> Will this trigger a pipeline to run the analysis?

No, this is only for checking policy to see if analysis should be scheduled.

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:87: pass
On 2016/09/13 16:37:31, stgao (slow) wrote:
> Will this result int error later in the code below?

Done.

https://codereview.chromium.org/2299883005/diff/100001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:123:
app_identity.get_default_version_hostname(), analysis.key.urlsafe())
On 2016/09/13 16:37:31, stgao (slow) wrote:
> Should we move the hostname part to common/appengine_util.py?

Done.

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

lijeffrey@chromium.org changed reviewers:
+ lijeffrey@chromium.org

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

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
File appengine/findit/crash/findit_for_client.py (right):

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:9: and can be refered to as
chromecrash."""
nit: referred

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:30: """Checks if the args pass the
client policy or not, update the parameters
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: ...
  ...
"""

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:38: customized_data.get('channel'),
[]):
split out customized_data.get('channel') since it's being used in the logging
too, and makes this line a bit more readable

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:61: # TODO(katesonia): Add
clusterfuzz policy check.
nit: I would move this todo to inside the if statement like you have with
GetAnalysisForClient

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:85: """Resets analysis and returns
if analysis get successfully reset or not."""
nit: """... and returns whether analysis was successfully reset."""

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
File appengine/findit/crash/type_enums.py (right):

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/type_enums.py:19: CRACAS = 'crascas'
nit: cracas?

[Martin Barbella, 2016-09-17 23:20:34]

mbarbella@chromium.org changed reviewers:
+ mbarbella@chromium.org

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

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:89: client_config =
getattr(CrashConfig.Get(), client_id)
This feels pretty hacky. If this is a common pattern, CrashConfig or
VersionedConfig (preferably the latter) should provide helpers to actually get
values.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:90: topic =
client_config['analysis_result_pubsub_topic']
Using strings in these places is very error prone. I'd really like to see this
config code cleaned up a bit, but it doesn't have to be in this CL.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:130: crash_identifiers, chrome_version,
signature,
Nit: this formatting looks a little wonky. Could you pyformat this?

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
File appengine/findit/crash/findit_for_client.py (right):

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:36: if platform not in
crash_config.fracas.get(
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.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:86: if not analysis:
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.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:129: if (analysis.client_id ==
CrashClient.FRACAS or
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:32:35]

Patchset #5 (id:160001) has been deleted

[Sharu Jiang, 2016-09-19 21:32:41]

Patchset #5 (id:180001) has been deleted

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

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:89: client_config =
getattr(CrashConfig.Get(), client_id)
On 2016/09/17 23:20:35, Martin Barbella wrote:
> This feels pretty hacky. If this is a common pattern, CrashConfig or
> VersionedConfig (preferably the latter) should provide helpers to actually get
> values.

Done.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:90: topic =
client_config['analysis_result_pubsub_topic']
On 2016/09/17 23:20:35, Martin Barbella wrote:
> Using strings in these places is very error prone. I'd really like to see this
> config code cleaned up a bit, but it doesn't have to be in this CL.

Add a TODO, will address this in another cl.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:130: crash_identifiers, chrome_version,
signature,
On 2016/09/17 23:20:35, Martin Barbella wrote:
> Nit: this formatting looks a little wonky. Could you pyformat this?

In order to avoid this cl growing too big, I will do this in a separate
follow-up cl to pyformat findit :)

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
File appengine/findit/crash/findit_for_client.py (right):

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:9: and can be refered to as
chromecrash."""
On 2016/09/16 16:28:56, lijeffrey wrote:
> nit: referred

Done.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:30: """Checks if the args pass the
client policy or not, update the parameters
On 2016/09/16 16:28:56, lijeffrey wrote:
> 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: ...
>   ...
> """

Done.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:36: if platform not in
crash_config.fracas.get(
On 2016/09/17 23:20:35, Martin Barbella wrote:
> 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.

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

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:38: customized_data.get('channel'),
[]):
On 2016/09/16 16:28:56, lijeffrey wrote:
> split out customized_data.get('channel') since it's being used in the logging
> too, and makes this line a bit more readable

Done.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:61: # TODO(katesonia): Add
clusterfuzz policy check.
On 2016/09/16 16:28:55, lijeffrey wrote:
> nit: I would move this todo to inside the if statement like you have with
> GetAnalysisForClient

Done.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:85: """Resets analysis and returns
if analysis get successfully reset or not."""
On 2016/09/16 16:28:56, lijeffrey wrote:
> nit: """... and returns whether analysis was successfully reset."""

Done.

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:86: if not analysis:
On 2016/09/17 23:20:35, Martin Barbella wrote:
> 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.

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 :)

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:129: if (analysis.client_id ==
CrashClient.FRACAS or
On 2016/09/17 23:20:35, Martin Barbella wrote:
> 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.

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

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
File appengine/findit/crash/type_enums.py (right):

https://codereview.chromium.org/2299883005/diff/120001/appengine/findit/crash...
appengine/findit/crash/type_enums.py:19: CRACAS = 'crascas'
On 2016/09/16 16:28:56, lijeffrey wrote:
> nit: cracas?

Ouch, done.

[Martin Barbella, 2016-09-19 21:42:37]

lgtm

https://codereview.chromium.org/2299883005/diff/140001/appengine/findit/crash...
File appengine/findit/crash/findit_for_client.py (right):

https://codereview.chromium.org/2299883005/diff/140001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:82: def
CreateAnalysisForClient(crash_identifiers, client_id):
Nit: missing docstring on this one.

[Sharu Jiang, 2016-09-19 23:30:39]

https://codereview.chromium.org/2299883005/diff/140001/appengine/findit/crash...
File appengine/findit/crash/findit_for_client.py (right):

https://codereview.chromium.org/2299883005/diff/140001/appengine/findit/crash...
appengine/findit/crash/findit_for_client.py:82: def
CreateAnalysisForClient(crash_identifiers, client_id):
On 2016/09/19 21:42:37, Martin Barbella wrote:
> Nit: missing docstring on this one.

Done.

[stgao, 2016-09-20 23:00:10]

lgtm with nits.

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/commo...
File appengine/findit/common/constants.py (right):

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/commo...
appengine/findit/common/constants.py:12: CRASH_BACKEND = {'fracas':
'crash-backend-fracas',
style nit: usually we do below. Is this the recommended format of pyformat?
---------------------
var = {
  'key': value,
}
---------------------

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/crash...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:124:
findit_for_client.SetAnalysis(analysis, chrome_version, signature, client_id,
Reset Analysis or Update Analysis?

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:147:
constants.CRASH_BACKEND[client_id])
Assert client id is supported at the very beginning?

[Sharu Jiang, 2016-09-21 18:06:03]

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/commo...
File appengine/findit/common/constants.py (right):

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/commo...
appengine/findit/common/constants.py:12: CRASH_BACKEND = {'fracas':
'crash-backend-fracas',
On 2016/09/20 23:00:10, stgao (slow) wrote:
> style nit: usually we do below. Is this the recommended format of pyformat?
> ---------------------
> var = {
>   'key': value,
> }
> ---------------------

Noop, done, I will do a pyformat cl after all of pending cls get committed.

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/crash...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:124:
findit_for_client.SetAnalysis(analysis, chrome_version, signature, client_id,
On 2016/09/20 23:00:10, stgao (slow) wrote:
> Reset Analysis or Update Analysis?

Done.

https://codereview.chromium.org/2299883005/diff/200001/appengine/findit/crash...
appengine/findit/crash/crash_pipeline.py:147:
constants.CRASH_BACKEND[client_id])
On 2016/09/20 23:00:10, stgao (slow) wrote:
> Assert client id is supported at the very beginning?

Added a check in findit_for_client.CheckPolicyForClient so crash_pipeline don't
need to know about client :)

[Sharu Jiang, 2016-09-21 18:15:12]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-09-21 18:15:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@chromium.org, stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2299883005/#ps220001
(title: "Fix nits.")

[commit-bot: I haz the power, 2016-09-21 18:15:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sharu Jiang, 2016-09-21 18:15:35]

The CQ bit was unchecked by katesonia@chromium.org

[Sharu Jiang, 2016-09-21 18:28:15]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-09-21 18:28:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@chromium.org, stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2299883005/#ps240001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-09-21 18:28:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 18:42:44]

Description was changed from

==========
[Findit] Add findit_for_client to do analysis based on client_id

Add findit_for_client on top of findit_for_fracas and findit_for_clusterfuzz(to
be implemented) to do client-specific checking and call different module to do
analysis based on client_id.

Doc:
https://docs.google.com/a/google.com/document/d/1XkGjz7O2WNoGz2_x2JpbaCFZKsKq...

BUG=645203
==========

to

==========
[Findit] Add findit_for_client to do analysis based on client_id

Add findit_for_client on top of findit_for_fracas and findit_for_clusterfuzz(to
be implemented) to do client-specific checking and call different module to do
analysis based on client_id.

Doc:
https://docs.google.com/a/google.com/document/d/1XkGjz7O2WNoGz2_x2JpbaCFZKsKq...

BUG=645203

Committed:
https://chromium.googlesource.com/infra/infra/+/bdee0f60631fbffc47f4ca7c58b9b...
==========

[commit-bot: I haz the power, 2016-09-21 18:42:46]

Committed patchset #7 (id:240001) as
https://chromium.googlesource.com/infra/infra/+/bdee0f60631fbffc47f4ca7c58b9b...