[Findit] Modify Findit API to return more information to Sheriff-O-Matic.

[Findit] Modify Findit API to return more information to Sheriff-O-Matic.

1. Add a flag (is_flaky_test) to mark if the result is for flaky test.
2. Add try_job_status.
3. Add confidence score for each suspected_cl.
4. Add analysis_approach for each suspected_cl.

BUG=609240, 655234
Committed: https://chromium.googlesource.com/infra/infra/+/3b1a71cedd4d149ed646ce175b51ee03fd9908ee

[chanli, 2016-10-18 20:50:47]

chanli@chromium.org changed reviewers:
+ lijeffrey@chromium.org, stgao@chromium.org

[chanli, 2016-10-18 20:50:47]

ptal

[stgao, 2016-10-18 21:06:22]

I will review this later today. But I have a question: will this API change
cause breakage on alerts-dispatcher or SoM side?

[stgao, 2016-10-19 00:43:08]

Looks pretty good.

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
File appengine/findit/findit_api.py (right):

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:75: class _TryJobStatus(messages.Enum):
More status here:
1. not supported
2. pending
3. error when finished
4. no culprit found when finished

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:293:
results.append(self._GenerateBuildFailureAnalysisResult(
Do we have an estimation of how many ndb read in order to generate one result?

[chanli, 2016-10-19 03:00:59]

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
File appengine/findit/findit_api.py (right):

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:75: class _TryJobStatus(messages.Enum):
On 2016/10/19 00:43:08, stgao wrote:
> More status here:
> 1. not supported
> 2. pending
> 3. error when finished
> 4. no culprit found when finished

I was thinking the try job status is to let sheriffs know should they wait for
try job results or not. That's why I only have 2 status. If the message showed
on SoM are the same for the status you mentioned, maybe we don't need to add the
status you mentioned.

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:293:
results.append(self._GenerateBuildFailureAnalysisResult(
On 2016/10/19 00:43:08, stgao wrote:
> Do we have an estimation of how many ndb read in order to generate one result?

For cases with the most reads:
1. 1 ndb read for confidence which can be shared by all results
2. 1 ndb read for WfAnalysis which will be shared by all results for this
analysis
For each step/test
3. 2 ndb reads for WfSwarmingTasks - first check if flaky, second check for
ref_name (can be improved)
3. 1 ndb read for WfTryJob
4. x ndb reads for WfSuspectedCL (based on how many suspected cls, normally
should be 1)

So  >5 reads.

There are some obvious improvements:
1. reduce the number of WfSwarmingTask reads (not always 1, based on the first
failures) by sharing all tasks among results of the same analysis
2. reduce the number of WfTryJob reads - same approach as WfSwarmingTask
3. Reduce the number of WfSuspectedCL reads -maybe need to handle try-job
results for non-first-time-failure differently.

Since there are several places need to be improved, I'll implement them in a
separate CL.

[stgao, 2016-10-21 01:51:22]

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
File appengine/findit/findit_api.py (right):

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:75: class _TryJobStatus(messages.Enum):
On 2016/10/19 03:00:59, chanli wrote:
> On 2016/10/19 00:43:08, stgao wrote:
> > More status here:
> > 1. not supported
> > 2. pending
> > 3. error when finished
> > 4. no culprit found when finished
> 
> I was thinking the try job status is to let sheriffs know should they wait for
> try job results or not. That's why I only have 2 status. If the message showed
> on SoM are the same for the status you mentioned, maybe we don't need to add
the
> status you mentioned.

OK, I agree 2-4 could be merged into RUNNING or FINISHED.
But for case 1, how do we expect sheriff to know it is not supported, and don't
wait?

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:293:
results.append(self._GenerateBuildFailureAnalysisResult(
On 2016/10/19 03:00:59, chanli wrote:
> On 2016/10/19 00:43:08, stgao wrote:
> > Do we have an estimation of how many ndb read in order to generate one
result?
> 
> For cases with the most reads:
> 1. 1 ndb read for confidence which can be shared by all results
> 2. 1 ndb read for WfAnalysis which will be shared by all results for this
> analysis
> For each step/test
> 3. 2 ndb reads for WfSwarmingTasks - first check if flaky, second check for
> ref_name (can be improved)
> 3. 1 ndb read for WfTryJob
> 4. x ndb reads for WfSuspectedCL (based on how many suspected cls, normally
> should be 1)
> 
> So  >5 reads.
> 
> There are some obvious improvements:
> 1. reduce the number of WfSwarmingTask reads (not always 1, based on the first
> failures) by sharing all tasks among results of the same analysis
> 2. reduce the number of WfTryJob reads - same approach as WfSwarmingTask
> 3. Reduce the number of WfSuspectedCL reads -maybe need to handle try-job
> results for non-first-time-failure differently.
> 
> Since there are several places need to be improved, I'll implement them in a
> separate CL.

Acknowledged.

[stgao, 2016-10-22 00:24:15]

lgtm

Per discussion, we should hold the CL until SoM is changed to be ready for the
new data if SoM could be broken without change.

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
File appengine/findit/findit_api.py (right):

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:75: class _TryJobStatus(messages.Enum):
On 2016/10/21 01:51:22, stgao wrote:
> On 2016/10/19 03:00:59, chanli wrote:
> > On 2016/10/19 00:43:08, stgao wrote:
> > > More status here:
> > > 1. not supported
> > > 2. pending
> > > 3. error when finished
> > > 4. no culprit found when finished
> > 
> > I was thinking the try job status is to let sheriffs know should they wait
for
> > try job results or not. That's why I only have 2 status. If the message
showed
> > on SoM are the same for the status you mentioned, maybe we don't need to add
> the
> > status you mentioned.
> 
> OK, I agree 2-4 could be merged into RUNNING or FINISHED.
> But for case 1, how do we expect sheriff to know it is not supported, and
don't
> wait?

Per discussion offline, we will go with the current approach, and make needed
change later.

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:258: swarming_task =
WfSwarmingTask.Get(*try_job_key.split('/'),
Do we have function for the encode and decode of key like this one?

[chanli, 2016-10-24 18:30:44]

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
File appengine/findit/findit_api.py (right):

https://codereview.chromium.org/2425853005/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:258: swarming_task =
WfSwarmingTask.Get(*try_job_key.split('/'),
On 2016/10/22 00:24:14, stgao wrote:
> Do we have function for the encode and decode of key like this one?

Done.

[chanli, 2016-10-28 01:12:46]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-10-28 01:12:47]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2425853005/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-28 01:12:58]

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-10-28 02:15:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 02:15:31]

Try jobs failed on following builders:
  Infra Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3221c72c6db82310)

[chanli, 2016-10-28 02:20:50]

The CQ bit was checked by chanli@chromium.org

[commit-bot: I haz the power, 2016-10-28 02:21:00]

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-10-28 02:29:58]

Description was changed from

==========
[Findit] Modify Findit API to return more information to Sheriff-O-Matic.

1. Add a flag (is_flaky_test) to mark if the result is for flaky test.
2. Add try_job_status.
3. Add confidence score for each suspected_cl.
4. Add analysis_approach for each suspected_cl.

BUG=609240, 655234
==========

to

==========
[Findit] Modify Findit API to return more information to Sheriff-O-Matic.

1. Add a flag (is_flaky_test) to mark if the result is for flaky test.
2. Add try_job_status.
3. Add confidence score for each suspected_cl.
4. Add analysis_approach for each suspected_cl.

BUG=609240, 655234

Committed:
https://chromium.googlesource.com/infra/infra/+/3b1a71cedd4d149ed646ce175b51e...
==========

[commit-bot: I haz the power, 2016-10-28 02:30:00]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/3b1a71cedd4d149ed646ce175b51e...