[Findit] Integrate with Fracas through Pub/Sub.

[Findit] Integrate with Fracas through Pub/Sub.

Main changes are:
1. Add a new frontend to handle crashes, and update dispatching rule accordingly.
2. Add a new backend and a new task queue to run analysis of crashes from Fracas.
3. Add crash configration(NDB module, handler, and UI template).
4. Add a handler to accept Pub/Sub push of Fracas messages.
5. Add the NDB model to save Fracas crashes and the analysis result.
6. Add the pipeline to run the analysis and publish results to a configured topic.
7. Update util_scripts/run.sh to set the app id through ENV for testing deployment.

A demo client app to mimic Fracas is in https://chromereviews.googleplex.com/392327013/.

The implementation is according to design doc https://docs.google.com/a/google.com/document/d/1fDhJ2suyTsCLXukJwNDqtTAx_hK0hjHrOuUFFR_jDtE/edit?usp=sharing

BUG=600460
Committed: https://chromium.googlesource.com/infra/infra/+/0c1485443c65aeab3e432bb86ecbe583cb7bb989

[stgao, 2016-04-04 18:56:25]

Description was changed from

==========
[Findit] Integrate with Fracas through Pub/Sub.

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

to

==========
[Findit] Integrate with Fracas through Pub/Sub.

BUG=600460
==========

[stgao, 2016-04-06 18:50:01]

Patchset #2 (id:20001) has been deleted

[stgao, 2016-04-07 19:14:48]

Patchset #4 (id:80001) has been deleted

[stgao, 2016-04-07 19:22:09]

Description was changed from

==========
[Findit] Integrate with Fracas through Pub/Sub.

BUG=600460
==========

to

==========
[Findit] Integrate with Fracas through Pub/Sub.

Main changes are:
1. Add a new frontend to handle crashes, and update dispatching rule
accordingly.
2. Add a new backend and a new task queue to run analysis of crashes from
Fracas.
3. Add crash configration(NDB module, handler, and UI template).
4. Add a handler to accept Pub/Sub push of Fracas messages.
5. Add the NDB model to save Fracas crashes and the analysis result.
6. Add the pipeline to run the analysis and publish results to a configured
topic.
7. Update util_scripts/run.sh to set the app id through ENV for testing
deployment.

A demo client app to mimic Fracas is in
https://chromereviews.googleplex.com/392327013/.

BUG=600460
==========

[stgao, 2016-04-07 19:22:47]

Description was changed from

==========
[Findit] Integrate with Fracas through Pub/Sub.

Main changes are:
1. Add a new frontend to handle crashes, and update dispatching rule
accordingly.
2. Add a new backend and a new task queue to run analysis of crashes from
Fracas.
3. Add crash configration(NDB module, handler, and UI template).
4. Add a handler to accept Pub/Sub push of Fracas messages.
5. Add the NDB model to save Fracas crashes and the analysis result.
6. Add the pipeline to run the analysis and publish results to a configured
topic.
7. Update util_scripts/run.sh to set the app id through ENV for testing
deployment.

A demo client app to mimic Fracas is in
https://chromereviews.googleplex.com/392327013/.

BUG=600460
==========

to

==========
[Findit] Integrate with Fracas through Pub/Sub.

Main changes are:
1. Add a new frontend to handle crashes, and update dispatching rule
accordingly.
2. Add a new backend and a new task queue to run analysis of crashes from
Fracas.
3. Add crash configration(NDB module, handler, and UI template).
4. Add a handler to accept Pub/Sub push of Fracas messages.
5. Add the NDB model to save Fracas crashes and the analysis result.
6. Add the pipeline to run the analysis and publish results to a configured
topic.
7. Update util_scripts/run.sh to set the app id through ENV for testing
deployment.

A demo client app to mimic Fracas is in
https://chromereviews.googleplex.com/392327013/.

The implementation is according to design doc
https://docs.google.com/a/google.com/document/d/1fDhJ2suyTsCLXukJwNDqtTAx_hK0...

BUG=600460
==========

[stgao, 2016-04-07 19:25:34]

stgao@chromium.org changed reviewers:
+ jchinlee@chromium.org, katesonia@chromium.org, lijeffrey@chromium.org,
mbarbella@google.com, mimee@chromium.org

[stgao, 2016-04-07 19:25:35]

Hi forks,

Do you mind a review of this CL for the integration with Fracas through Pub/Sub?

A demo client app to mimic Fracas is in
https://chromereviews.googleplex.com/392327013/.

Thanks,
Shuotao Gao

[stgao, 2016-04-07 20:18:59]

stgao@chromium.org changed reviewers:
+ mbarbella@chromium.org
- mbarbella@google.com

[mimee, 2016-04-07 23:43:10]

I only managed to skim through these.

Marty and Jao-ke will also take a look at some point.

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

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas.py:9: channel, platform, signature, stack_trace,
crashed_version, cpm):
cpm->cpms

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas.py:27: "dep_short_name": "v8",  # A short name to
represent the dependency.
Why dep_short_name when there is suspected_dep?
Can we keep just one?

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas.py:34: "dep_short_name": "v8",
Why is there a whole set of those under culprit again?

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

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:112: # A new analysis is not
needed if last one didn't complete and fail.
fail-> failed.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:116: elif not analysis:
Already returned previously, elif->if

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/handl...
File appengine/findit/handlers/crash/fracas_crash.py (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/handl...
appengine/findit/handlers/crash/fracas_crash.py:39: crash_data['channel'],
crash_data['platform'], crash_data['signature'],
I prefer passing the object and unpacking it in the implementation. Or specify
that you only take a particular json form. Also, we use just 'version', not
'chrome_version'. I think Chrome is implied.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/main.py
File appengine/findit/main.py (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/main....
appengine/findit/main.py:73: # This is for the "waterfall-backend" module.
Omitting 'This is for the ' from comments would be perfectly fine.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/model...
File appengine/findit/model/crash/crash_config.py (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:18: #   "supported_channels": {
This looks more like supported_platform_list_by_channel.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:22: fracas =
ndb.JsonProperty(indexed=False, default={})
newline EOF

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/templ...
File appengine/findit/templates/crash/crash_config.html (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/templ...
appengine/findit/templates/crash/crash_config.html:4: <script
src="//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
This might just be a sample internal page, but jquery is too heavy for this. Not
using jquery is about the same number of lines of code.

Also, why is script in <head>?

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/third...
File appengine/findit/third_party/uritemplate (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/third...
appengine/findit/third_party/uritemplate:1:
../../third_party/uritemplate/uritemplate/
I am not very familiar with this practice. Is this generated code? Specifying
lib directory in appengine_config.py is very simple.

[stgao, 2016-04-08 01:14:08]

Thanks for the quick review.

Marty, Jao-ke, and Sharu, do you mind a reviewing?

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

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas.py:9: channel, platform, signature, stack_trace,
crashed_version, cpm):
On 2016/04/07 23:43:09, mimee wrote:
> cpm->cpms

Done.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas.py:27: "dep_short_name": "v8",  # A short name to
represent the dependency.
On 2016/04/07 23:43:09, mimee wrote:
> Why dep_short_name when there is suspected_dep?
> Can we keep just one?

I'd keep it. It is more for display on UI.
suspected_dep could be quite long (third_party/bla/bla), while dep_short_name is
more concise.

If you are sure that the longer version will be show, we could remove it.

To be consistent with what have for ClusterFuzz, I change dep to project.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas.py:34: "dep_short_name": "v8",
On 2016/04/07 23:43:09, mimee wrote:
> Why is there a whole set of those under culprit again?

This is for individual CL. Culprits won't be always in the same dep.
This is the same as ClusterFuzz.

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

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:112: # A new analysis is not
needed if last one didn't complete and fail.
On 2016/04/07 23:43:09, mimee wrote:
> fail-> failed.

Corrected.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:116: elif not analysis:
On 2016/04/07 23:43:09, mimee wrote:
> Already returned previously, elif->if

Done.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/handl...
File appengine/findit/handlers/crash/fracas_crash.py (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/handl...
appengine/findit/handlers/crash/fracas_crash.py:39: crash_data['channel'],
crash_data['platform'], crash_data['signature'],
On 2016/04/07 23:43:09, mimee wrote:
> I prefer passing the object and unpacking it in the implementation. Or specify
> that you only take a particular json form.

I'd unpack the json here to prevent malformed data from propagating  all the way
to the backend.

Updated the documentation here.

> Also, we use just 'version', not
> 'chrome_version'. I think Chrome is implied.
I'd make it explicit, because dependencies (like v8, pdfium, etc) also have
version or revisions.
We might need an clankium_version later for Chrome on Android.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/main.py
File appengine/findit/main.py (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/main....
appengine/findit/main.py:73: # This is for the "waterfall-backend" module.
On 2016/04/07 23:43:09, mimee wrote:
> Omitting 'This is for the ' from comments would be perfectly fine.

sounds good.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/model...
File appengine/findit/model/crash/crash_config.py (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:18: #   "supported_channels": {
On 2016/04/07 23:43:09, mimee wrote:
> This looks more like supported_platform_list_by_channel.

Done.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:22: fracas =
ndb.JsonProperty(indexed=False, default={})
On 2016/04/07 23:43:09, mimee wrote:
> newline EOF

hm, that seems not the convention in infra repo.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/templ...
File appengine/findit/templates/crash/crash_config.html (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/templ...
appengine/findit/templates/crash/crash_config.html:4: <script
src="//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
On 2016/04/07 23:43:09, mimee wrote:
> This might just be a sample internal page, but jquery is too heavy for this.
Not
> using jquery is about the same number of lines of code.
We prefer jquery over native javascript functions & ajax for simplicity and
consistence within the Findit app.

> 
> Also, why is script in <head>?
What's the alternative?

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/third...
File appengine/findit/third_party/uritemplate (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/third...
appengine/findit/third_party/uritemplate:1:
../../third_party/uritemplate/uritemplate/
On 2016/04/07 23:43:09, mimee wrote:
> I am not very familiar with this practice. Is this generated code? Specifying
> lib directory in appengine_config.py is very simple.
These are links to avoid copy those codes. This is the convention in the infra
repo.

And I guess this committed CL https://codereview.chromium.org/1866053002/
resolved your doubt.

[mimee, 2016-04-08 15:36:04]

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

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas.py:27: "dep_short_name": "v8",  # A short name to
represent the dependency.
On 2016/04/08 01:14:08, stgao wrote:
> On 2016/04/07 23:43:09, mimee wrote:
> > Why dep_short_name when there is suspected_dep?
> > Can we keep just one?
> 
> I'd keep it. It is more for display on UI.
> suspected_dep could be quite long (third_party/bla/bla), while dep_short_name
is
> more concise.
> 
> If you are sure that the longer version will be show, we could remove it.
> 
> To be consistent with what have for ClusterFuzz, I change dep to project.

Would there be a review url, along with revision? I was told that people click
on the review anyway.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/templ...
File appengine/findit/templates/crash/crash_config.html (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/templ...
appengine/findit/templates/crash/crash_config.html:4: <script
src="//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
On 2016/04/08 01:14:08, stgao wrote:
> On 2016/04/07 23:43:09, mimee wrote:
> > This might just be a sample internal page, but jquery is too heavy for this.
> Not
> > using jquery is about the same number of lines of code.
> We prefer jquery over native javascript functions & ajax for simplicity and
> consistence within the Findit app.
> 
> > 
> > Also, why is script in <head>?
> What's the alternative?

The alternative is to put script on bottom because loading script blocks
parallel downloads and slows down the webpage, as per convention.

Although this is a case where the script modifies the core parts of the html
page (not desired behavior), so loading it faster earlier doesn't actually help
that much. It is better to not pass html code around though, if possible.

[Martin Barbella, 2016-04-08 18:10:24]

I want to do another pass on this, but some initial comments.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas.py:9: channel, platform, signature, stack_trace,
crashed_version, cpms):
cpms is still potentially confusing. Let's go with versions_to_cpm or something
like that.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:36: def
_SetErrorWhenAborted(self, aborted):
I don't think this necessarily needs to be in its own function, but as-is the
name is a bit confusing. When seems to imply a callback, so maybe IfAborted
would be clearer. It also doesn't seem like the aborted argument is necessary.

Also, minor nit, just return if not aborted.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:68: # Later, we might consider
adding arbitrary tags.
This seems ideal. At the very least, this should be a TODO.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:108: def _NeedANewAnalysis(
Nit: rename to _NeedsNewAnalysis.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:109: channel, platform,
signature, stack_trace, chrome_version, cpms):
Same comment about cpms from before.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:132: channel, platform,
signature, stack_trace, chrome_version, cpms,
Ditto.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/handl...
File appengine/findit/handlers/crash/fracas_crash.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/handl...
appengine/findit/handlers/crash/fracas_crash.py:40: if token !=
crash_config.fracas.get('crash_data_push_token'):
Is there a way to actually verify that these pushes are coming from Pub/Sub?
This is pretty far from ideal. If so, we should omit the token entirely.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/model...
File appengine/findit/model/crash/crash_analysis.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/model...
appengine/findit/model/crash/crash_analysis.py:14: crashed_version =
ndb.StringProperty(indexed=False)
Nit: whitespace between properties for readability.

[stgao, 2016-04-11 20:32:20]

Comments are all addressed.

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

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/crash...
appengine/findit/crash/fracas.py:27: "dep_short_name": "v8",  # A short name to
represent the dependency.
On 2016/04/08 15:36:03, mimee wrote:
> On 2016/04/08 01:14:08, stgao wrote:
> > On 2016/04/07 23:43:09, mimee wrote:
> > > Why dep_short_name when there is suspected_dep?
> > > Can we keep just one?
> > 
> > I'd keep it. It is more for display on UI.
> > suspected_dep could be quite long (third_party/bla/bla), while
dep_short_name
> is
> > more concise.
> > 
> > If you are sure that the longer version will be show, we could remove it.
> > 
> > To be consistent with what have for ClusterFuzz, I change dep to project.
> 
> Would there be a review url, along with revision? I was told that people click
> on the review anyway.

I guess you mean code review url. Yes, we could do that.
And in Findit for waterfall, actually we show use the code review url if it is
available.

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/templ...
File appengine/findit/templates/crash/crash_config.html (right):

https://codereview.chromium.org/1852383002/diff/100001/appengine/findit/templ...
appengine/findit/templates/crash/crash_config.html:4: <script
src="//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
On 2016/04/08 15:36:03, mimee wrote:
> On 2016/04/08 01:14:08, stgao wrote:
> > On 2016/04/07 23:43:09, mimee wrote:
> > > This might just be a sample internal page, but jquery is too heavy for
this.
> > Not
> > > using jquery is about the same number of lines of code.
> > We prefer jquery over native javascript functions & ajax for simplicity and
> > consistence within the Findit app.
> > 
> > > 
> > > Also, why is script in <head>?
> > What's the alternative?
> 
> The alternative is to put script on bottom because loading script blocks
> parallel downloads and slows down the webpage, as per convention.
Ah, I see. But if we move all scripts to the end of page, the user might see a
page with empty config values during page loading. Thus, I'd leave them in the
header.

> 
> Although this is a case where the script modifies the core parts of the html
> page (not desired behavior), so loading it faster earlier doesn't actually
help
> that much. It is better to not pass html code around though, if possible.

Only the values of textareas are passed in Javascript for better formatting. It
seems no obvious workarounds to me.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas.py:9: channel, platform, signature, stack_trace,
crashed_version, cpms):
On 2016/04/08 18:10:24, Martin Barbella wrote:
> cpms is still potentially confusing. Let's go with versions_to_cpm or
something
> like that.

Sg. Done.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:36: def
_SetErrorWhenAborted(self, aborted):
On 2016/04/08 18:10:24, Martin Barbella wrote:
> I don't think this necessarily needs to be in its own function, but as-is the
> name is a bit confusing. When seems to imply a callback, so maybe IfAborted
> would be clearer. It also doesn't seem like the aborted argument is necessary.
This function is necessary for unittest purpose with 100% code coverage. It's
more of a workaround. But we can't find a good way to simulate an abortion in
appengine pipeline.

> 
> Also, minor nit, just return if not aborted.
Done.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:68: # Later, we might consider
adding arbitrary tags.
On 2016/04/08 18:10:24, Martin Barbella wrote:
> This seems ideal. At the very least, this should be a TODO.

Maybe let's remove for now. The intention was to avoid adding too many fields to
the NDB model.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:108: def _NeedANewAnalysis(
On 2016/04/08 18:10:24, Martin Barbella wrote:
> Nit: rename to _NeedsNewAnalysis.

Done.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:109: channel, platform,
signature, stack_trace, chrome_version, cpms):
On 2016/04/08 18:10:24, Martin Barbella wrote:
> Same comment about cpms from before.

Done.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:132: channel, platform,
signature, stack_trace, chrome_version, cpms,
On 2016/04/08 18:10:24, Martin Barbella wrote:
> Ditto.

Done.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/handl...
File appengine/findit/handlers/crash/fracas_crash.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/handl...
appengine/findit/handlers/crash/fracas_crash.py:40: if token !=
crash_config.fracas.get('crash_data_push_token'):
On 2016/04/08 18:10:24, Martin Barbella wrote:
> Is there a way to actually verify that these pushes are coming from Pub/Sub?
> This is pretty far from ideal. If so, we should omit the token entirely.

This is the only solution for now. A better solution is in b/25788912 as
described in the design doc.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/model...
File appengine/findit/model/crash/crash_analysis.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/model...
appengine/findit/model/crash/crash_analysis.py:14: crashed_version =
ndb.StringProperty(indexed=False)
On 2016/04/08 18:10:24, Martin Barbella wrote:
> Nit: whitespace between properties for readability.

Sounds good. I also have separators for different groups of properties.

[Martin Barbella, 2016-04-11 21:46:32]

LGTM overall, just a few more comments. I didn't look too closely at the tests.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:68: # Later, we might consider
adding arbitrary tags.
On 2016/04/11 20:32:19, stgao wrote:
> On 2016/04/08 18:10:24, Martin Barbella wrote:
> > This seems ideal. At the very least, this should be a TODO.
> 
> Maybe let's remove for now. The intention was to avoid adding too many fields
to
> the NDB model.

There are certainly ways to do this without adding new fields to the model,
though.

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
appengine/findit/crash/fracas.py:41: "confidence": "high/medium/low/0.6",  #
Optional confidence score.
Have any decisions been made on this format yet? I don't like the idea of
including both a rating string and potentially a number, so listing one or the
other would be better here (it can be changed later).

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:35: class
AnalyzeCrashPipeline(FracasCrashBasePipeline):
Both of these class names are a bit confusing.

FracasBasePipeline and FracasAnalysisPipeline? I'm not entirely sure what other
cases FracasCrashBasePipeline would be used for in the future, so there might be
a better option.

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
File appengine/findit/crash/test/crash_testcase.py (right):

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:15: 'crash_data_push_token':
'secrete_token',
s/secrete/secret/

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/model...
File appengine/findit/model/crash/crash_config.py (right):

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:16: #   "crash_data_push_token":
"secrete_token",
s/secrete/secret/

[Sharu, 2016-04-12 00:23:13]

katesonia@google.com changed reviewers:
+ katesonia@google.com

[Sharu, 2016-04-12 00:23:14]

Sorry for the delay, was too busy with the demo last week.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas.py:62: # TODO (katesnonia): hook the analysis
logic up here.
Typo: katesonia

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:83: analysis =
FracasCrashAnalysis.Get(channel, platform, signature)
Should we set the self.channel, self.platform, self.signature somewhere?

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/model...
File appengine/findit/model/crash/crash_analysis.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/model...
appengine/findit/model/crash/crash_analysis.py:18: signature =
ndb.StringProperty(indexed=False)
signature is fracas specific, clusterfuzz doesn't have signature, something
similar is the crash state, which usually has 3 top crash functions.

So should we put this in subclass?

[stgao, 2016-04-12 18:19:18]

Comments are addressed.

If I don't receive more comments by end of day today, I will land it EOD and
address new comments in a follow-up CL.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:68: # Later, we might consider
adding arbitrary tags.
On 2016/04/11 21:46:31, Martin Barbella wrote:
> On 2016/04/11 20:32:19, stgao wrote:
> > On 2016/04/08 18:10:24, Martin Barbella wrote:
> > > This seems ideal. At the very least, this should be a TODO.
> > 
> > Maybe let's remove for now. The intention was to avoid adding too many
fields
> to
> > the NDB model.
> 
> There are certainly ways to do this without adding new fields to the model,
> though.

Yes, there should be. But I'd like to defer the implementation for that until we
need much more arbitrary tags for query and monitoring. Leave a TODO here.

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
appengine/findit/crash/fracas.py:41: "confidence": "high/medium/low/0.6",  #
Optional confidence score.
On 2016/04/11 21:46:32, Martin Barbella wrote:
> Have any decisions been made on this format yet? I don't like the idea of
> including both a rating string and potentially a number, so listing one or the
> other would be better here (it can be changed later).

No final decision yet. Let's go with a number for now.

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:35: class
AnalyzeCrashPipeline(FracasCrashBasePipeline):
On 2016/04/11 21:46:32, Martin Barbella wrote:
> Both of these class names are a bit confusing.
> 
> FracasBasePipeline and FracasAnalysisPipeline? 
I like these two names.

> I'm not entirely sure what other
> cases FracasCrashBasePipeline would be used for in the future, so there might
be
> a better option.

The base class is used to save the channel,platform,signature for retrieving the
FracasCrashAnalysis from datastore and avoid duplicate code in subclasses here.
I don't think it will be used for other purpose.

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
File appengine/findit/crash/test/crash_testcase.py (right):

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:15: 'crash_data_push_token':
'secrete_token',
On 2016/04/11 21:46:32, Martin Barbella wrote:
> s/secrete/secret/

oops, done.

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/model...
File appengine/findit/model/crash/crash_config.py (right):

https://codereview.chromium.org/1852383002/diff/130001/appengine/findit/model...
appengine/findit/model/crash/crash_config.py:16: #   "crash_data_push_token":
"secrete_token",
On 2016/04/11 21:46:32, Martin Barbella wrote:
> s/secrete/secret/

Done.

[stgao, 2016-04-12 18:27:36]

It seems I missed comments by katesonia@ in an earlier patchset.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas.py:62: # TODO (katesnonia): hook the analysis
logic up here.
On 2016/04/12 00:23:13, Sharu wrote:
> Typo: katesonia

Oops, sorry. Updated.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/crash...
appengine/findit/crash/fracas_crash_pipeline.py:83: analysis =
FracasCrashAnalysis.Get(channel, platform, signature)
On 2016/04/12 00:23:13, Sharu wrote:
> Should we set the self.channel, self.platform, self.signature somewhere?

self.channel, self.platform, self.signature are set in the constructor. And
channel,platform,signature will always be passed over to the function ``run`` in
a pipeline.

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/model...
File appengine/findit/model/crash/crash_analysis.py (right):

https://codereview.chromium.org/1852383002/diff/110001/appengine/findit/model...
appengine/findit/model/crash/crash_analysis.py:18: signature =
ndb.StringProperty(indexed=False)
On 2016/04/12 00:23:14, Sharu wrote:
> signature is fracas specific, clusterfuzz doesn't have signature, something
> similar is the crash state, which usually has 3 top crash functions.
> 
> So should we put this in subclass?

From the perspective of abstraction, we could treat crash state in ClusterFuzz
as a crash signature. So it's fine to leave as is.

[Sharu, 2016-04-12 18:53:27]

lgtm

[stgao, 2016-04-14 23:21:08]

The CQ bit was checked by stgao@chromium.org

[stgao, 2016-04-14 23:21:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@chromium.org, katesonia@google.com
Link to the patchset: https://codereview.chromium.org/1852383002/#ps210001
(title: "Just rebase.")

[commit-bot: I haz the power, 2016-04-14 23:21:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1852383002/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1852383002/210001

[commit-bot: I haz the power, 2016-04-14 23:25:17]

Description was changed from

==========
[Findit] Integrate with Fracas through Pub/Sub.

Main changes are:
1. Add a new frontend to handle crashes, and update dispatching rule
accordingly.
2. Add a new backend and a new task queue to run analysis of crashes from
Fracas.
3. Add crash configration(NDB module, handler, and UI template).
4. Add a handler to accept Pub/Sub push of Fracas messages.
5. Add the NDB model to save Fracas crashes and the analysis result.
6. Add the pipeline to run the analysis and publish results to a configured
topic.
7. Update util_scripts/run.sh to set the app id through ENV for testing
deployment.

A demo client app to mimic Fracas is in
https://chromereviews.googleplex.com/392327013/.

The implementation is according to design doc
https://docs.google.com/a/google.com/document/d/1fDhJ2suyTsCLXukJwNDqtTAx_hK0...

BUG=600460
==========

to

==========
[Findit] Integrate with Fracas through Pub/Sub.

Main changes are:
1. Add a new frontend to handle crashes, and update dispatching rule
accordingly.
2. Add a new backend and a new task queue to run analysis of crashes from
Fracas.
3. Add crash configration(NDB module, handler, and UI template).
4. Add a handler to accept Pub/Sub push of Fracas messages.
5. Add the NDB model to save Fracas crashes and the analysis result.
6. Add the pipeline to run the analysis and publish results to a configured
topic.
7. Update util_scripts/run.sh to set the app id through ENV for testing
deployment.

A demo client app to mimic Fracas is in
https://chromereviews.googleplex.com/392327013/.

The implementation is according to design doc
https://docs.google.com/a/google.com/document/d/1fDhJ2suyTsCLXukJwNDqtTAx_hK0...

BUG=600460

Committed:
https://chromium.googlesource.com/infra/infra/+/0c1485443c65aeab3e432bb86ecbe...
==========

[commit-bot: I haz the power, 2016-04-14 23:25:19]

Committed patchset #10 (id:210001) as
https://chromium.googlesource.com/infra/infra/+/0c1485443c65aeab3e432bb86ecbe...