[Findit] Flake Checker: Extract commit position and git hash and display to UI for each analyzed bu…

[Findit] Flake Checker: Extract commit position and git hash and display to UI for each analyzed build

Each analyzed build's commit position and git hash are now extracted, with links to the commits available in the mouse-over UI in the graph.

Sample UI: https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium.mac&builder_name=Mac10.11%20Tests&build_number=3982&step_name=browser_tests%20on%20Mac-10.11&test_name=ContentVerifierPolicyTest.PolicyCorruptedOnStartup

BUG=672338
Review-Url: https://codereview.chromium.org/2563383002
Committed: https://chromium.googlesource.com/infra/infra/+/e2172016df4fce6ce4963d978e36a9bca814b402

[lijeffrey, 2016-12-12 06:00:30]

Description was changed from

==========
[Findit] Flake Checker: Extract commit position and git hash and display to UI
for each analyzed build

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

to

==========
[Findit] Flake Checker: Extract commit position and git hash and display to UI
for each analyzed build

Each analyzed build's commit position and git hash are now extracted, with links
to the commits available in the mouse-over UI in the graph.

Sample UI:
https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/check-flake?key=ag9...

BUG=672338
==========

[lijeffrey, 2016-12-12 06:00:30]

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

[lijeffrey, 2016-12-12 06:02:48]

Sorry, still using rietveld (need to sync with vadim@ to get my @chromium
account working in gerrit).

PTAL when you have the chance :)

[chanli, 2016-12-12 14:18:56]

On 2016/12/12 06:02:48, lijeffrey wrote:
> Sorry, still using rietveld (need to sync with vadim@ to get my @chromium
> account working in gerrit).
> 
> PTAL when you have the chance :)

Codewise LGTM with nit.

But I think we should collect git hash for try job, why do we want to display
git hash(commit position actually) for each build?

For me, "Suspected flake git commit position" means this is the commit position
for the culprit CL but not the suspected build. I'm not sure if rewording will
solve this...

[chanli, 2016-12-12 14:19:05]

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/check_flake.py:106: else None)
Nit: indent

[stgao, 2016-12-12 20:05:18]

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

[stgao, 2016-12-12 20:05:18]

not lgtm (just to prevent accident commit)

More details in my comments below.

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/check_flake.py:41: build =
build_util.DownloadBuildData(master_name, builder_name, build_number)
This seems not good. What if there are 100s of data points?
In that case, too many HTTP requests have to be sent.

Instead of retrieving info during page rendering, we'd better retrieve at some
stage of the Swarming rerun pipeline, save the data in NDB together with the
DataPoint, and then here we just read them directly.

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/templates/...
File appengine/findit/templates/flake/result.html (right):

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/templates/...
appengine/findit/templates/flake/result.html:93: not_run_data.push([value[0],
100, value[2]]);
Here, we don't need it?
Better to be consistent though.

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/templates/...
appengine/findit/templates/flake/result.html:252: Suspected flake git commit
position: <a href="https://crrev.com/{{ suspected_flake.git_hash }}"
target="_blank">{{ suspected_flake.commit_position }}</a>
This looks misleading to users.

Instead, we could show the regression range as: (commit_position_of_previous
build, commit_position_of_this_build]

Be careful about the inclusion and exclusion.

[lijeffrey, 2016-12-14 02:35:45]

Description was changed from

==========
[Findit] Flake Checker: Extract commit position and git hash and display to UI
for each analyzed build

Each analyzed build's commit position and git hash are now extracted, with links
to the commits available in the mouse-over UI in the graph.

Sample UI:
https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/check-flake?key=ag9...

BUG=672338
==========

to

==========
[Findit] Flake Checker: Extract commit position and git hash and display to UI
for each analyzed build

Each analyzed build's commit position and git hash are now extracted, with links
to the commits available in the mouse-over UI in the graph.

Sample UI:
https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/check-flake?master_...

BUG=672338
==========

[lijeffrey, 2016-12-14 02:37:13]

comments addressed, ptal

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/check_flake.py:41: build =
build_util.DownloadBuildData(master_name, builder_name, build_number)
On 2016/12/12 20:05:18, stgao (slow on Monday) wrote:
> This seems not good. What if there are 100s of data points?
> In that case, too many HTTP requests have to be sent.
> 
> Instead of retrieving info during page rendering, we'd better retrieve at some
> stage of the Swarming rerun pipeline, save the data in NDB together with the
> DataPoint, and then here we just read them directly.

Done.

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/check_flake.py:106: else None)
On 2016/12/12 14:19:05, chanli wrote:
> Nit: indent

Done.

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/templates/...
File appengine/findit/templates/flake/result.html (right):

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/templates/...
appengine/findit/templates/flake/result.html:93: not_run_data.push([value[0],
100, value[2]]);
On 2016/12/12 20:05:18, stgao (slow on Monday) wrote:
> Here, we don't need it?
> Better to be consistent though.

Done.

https://codereview.chromium.org/2563383002/diff/1/appengine/findit/templates/...
appengine/findit/templates/flake/result.html:252: Suspected flake git commit
position: <a href="https://crrev.com/{{ suspected_flake.git_hash }}"
target="_blank">{{ suspected_flake.commit_position }}</a>
On 2016/12/12 20:05:18, stgao (slow on Monday) wrote:
> This looks misleading to users.
> 
> Instead, we could show the regression range as: (commit_position_of_previous
> build, commit_position_of_this_build]
> 
> Be careful about the inclusion and exclusion.

Done.

[chanli, 2016-12-14 03:41:05]

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/check_flake.py:30: return None  # pragma: no
cover. This should not happen.
Would you mind add a test for this? Should be very easy to test

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
File appengine/findit/templates/flake/result.html (right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:184: if
(previousBuildCommitPosition && previousBuildGitHash && thisBuildCommitPosition
&& thisBuildGitHash) {
I think if we have git hashes but not commit positions, we should still display
the ranges, but with shortened git hashes. WDYT?

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:185: tooltipStr += '<br>Range: <a
href="https://crrev.com/' + previousBuildGitHash + '..' + thisBuildGitHash +
'"target="_blank">' + '(' + previousBuildCommitPosition + ':' +
thisBuildCommitPosition + ']</a>';
Nit: Range: (rev1:rev2]

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:264: Regression range: <a
href="https://crrev.com/{{ suspected_flake.git_hash }}..{{
suspected_flake.previous_git_hash }}" target="_blank">({{
suspected_flake.commit_position }}:{{ suspected_flake.previous_commit_position
}}]</a>
Nit: Range: (rev1:rev2]

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:30: if
build_number < 1:
Since build_number can be 0, shouldn't this be if build_number < 0?

[stgao, 2016-12-14 07:58:28]

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/check_flake.py:22: SWARMING_TASK_BASE_URL =
'https://chromium-swarm.appspot.com'
You may want to pull in my change and rebase.
This was gone.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/model/...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:25: previous_git_hash =
ndb.StringProperty(indexed=False)
It is not clear what "previous" means here.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
File appengine/findit/templates/flake/result.html (right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:98: not_run_data.push([value[0],
100, value[2], value[3], value[4], value[5], value[6]]);
Could we use commit position as X axis now? Later when data point from try-job
is added, we need commit position instead of build number. Better to get
prepared now.

BTW, we might need a script to update legacy data for those commit position and
git hash.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:182: var tooltipStr = 'Build #: <a
href="https://build.chromium.org/p/' + findit.masterName + '/builders/' +
findit.builderName + '/builds/' + buildNumber + '" target="_blank">' +
buildNumber + '</a>';
For the order of info entries, how about below?

Pass rate: XX%
Range: last:current
Build #: ...
Swarming task: ...

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:264: Regression range: <a
href="https://crrev.com/{{ suspected_flake.git_hash }}..{{
suspected_flake.previous_git_hash }}" target="_blank">({{
suspected_flake.commit_position }}:{{ suspected_flake.previous_commit_position
}}]</a>
Should the commit position and git hash switch in order? The link seems not
working in the demo UI.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:264: Regression range: <a
href="https://crrev.com/{{ suspected_flake.git_hash }}..{{
suspected_flake.previous_git_hash }}" target="_blank">({{
suspected_flake.commit_position }}:{{ suspected_flake.previous_commit_position
}}]</a>
nit: "(" and "]" could be removed.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:47:
last_commit_message)
Could we reuse
https://cs.chromium.org/chromium/infra/appengine/findit/waterfall/buildbot.py...
instead for #38-47?

I think  the commit position is in property "got_revision_cp".

[lijeffrey, 2016-12-14 22:15:11]

comments addressed, ptal

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/handle...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/check_flake.py:22: SWARMING_TASK_BASE_URL =
'https://chromium-swarm.appspot.com'
On 2016/12/14 07:58:28, stgao (slow on Monday) wrote:
> You may want to pull in my change and rebase.
> This was gone.

Done.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/handle...
appengine/findit/handlers/flake/check_flake.py:30: return None  # pragma: no
cover. This should not happen.
On 2016/12/14 03:41:05, chanli wrote:
> Would you mind add a test for this? Should be very easy to test

Done.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/model/...
File appengine/findit/model/flake/master_flake_analysis.py (right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/model/...
appengine/findit/model/flake/master_flake_analysis.py:25: previous_git_hash =
ndb.StringProperty(indexed=False)
On 2016/12/14 07:58:28, stgao (slow on Monday) wrote:
> It is not clear what "previous" means here.

Done.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
File appengine/findit/templates/flake/result.html (right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:98: not_run_data.push([value[0],
100, value[2], value[3], value[4], value[5], value[6]]);
On 2016/12/14 07:58:28, stgao (slow on Monday) wrote:
> Could we use commit position as X axis now? Later when data point from try-job
> is added, we need commit position instead of build number. Better to get
> prepared now.
> 
> BTW, we might need a script to update legacy data for those commit position
and
> git hash.

Done.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:182: var tooltipStr = 'Build #: <a
href="https://build.chromium.org/p/' + findit.masterName + '/builders/' +
findit.builderName + '/builds/' + buildNumber + '" target="_blank">' +
buildNumber + '</a>';
On 2016/12/14 07:58:28, stgao (slow on Monday) wrote:
> For the order of info entries, how about below?
> 
> Pass rate: XX%
> Range: last:current
> Build #: ...
> Swarming task: ...

Done.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:184: if
(previousBuildCommitPosition && previousBuildGitHash && thisBuildCommitPosition
&& thisBuildGitHash) {
On 2016/12/14 03:41:05, chanli wrote:
> I think if we have git hashes but not commit positions, we should still
display
> the ranges, but with shortened git hashes. WDYT?

They should come together as far as I can tell. This branch is for the case
build_number is 0 where there is no previous commit position/git hash

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:185: tooltipStr += '<br>Range: <a
href="https://crrev.com/' + previousBuildGitHash + '..' + thisBuildGitHash +
'"target="_blank">' + '(' + previousBuildCommitPosition + ':' +
thisBuildCommitPosition + ']</a>';
On 2016/12/14 03:41:05, chanli wrote:
> Nit: Range: (rev1:rev2]

I think this should already do that?

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:264: Regression range: <a
href="https://crrev.com/{{ suspected_flake.git_hash }}..{{
suspected_flake.previous_git_hash }}" target="_blank">({{
suspected_flake.commit_position }}:{{ suspected_flake.previous_commit_position
}}]</a>
On 2016/12/14 07:58:28, stgao (slow on Monday) wrote:
> nit: "(" and "]" could be removed.

Done.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/templa...
appengine/findit/templates/flake/result.html:264: Regression range: <a
href="https://crrev.com/{{ suspected_flake.git_hash }}..{{
suspected_flake.previous_git_hash }}" target="_blank">({{
suspected_flake.commit_position }}:{{ suspected_flake.previous_commit_position
}}]</a>
On 2016/12/14 07:58:28, stgao (slow on Monday) wrote:
> Should the commit position and git hash switch in order? The link seems not
> working in the demo UI.

Done.

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/waterf...
File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:30: if
build_number < 1:
On 2016/12/14 03:41:05, chanli wrote:
> Since build_number can be 0, shouldn't this be if build_number < 0?

You are absolutely right, good catch!

https://codereview.chromium.org/2563383002/diff/20001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:47:
last_commit_message)
On 2016/12/14 07:58:28, stgao (slow on Monday) wrote:
> Could we reuse
>
https://cs.chromium.org/chromium/infra/appengine/findit/waterfall/buildbot.py...
> instead for #38-47?
> 
> I think  the commit position is in property "got_revision_cp".

Done.

[stgao, 2016-12-14 23:33:37]

lgtm with a nit.

https://codereview.chromium.org/2563383002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2563383002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:109:
master_name, builder_name, build_number - 1)
Should we check whether build_number is 0 before -1 here?

[lijeffrey, 2016-12-15 06:38:17]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-12-15 06:38:17]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org,
stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2563383002/#ps60001
(title: "Fixing nit")

[lijeffrey, 2016-12-15 06:38:26]

https://codereview.chromium.org/2563383002/diff/40001/appengine/findit/waterf...
File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py
(right):

https://codereview.chromium.org/2563383002/diff/40001/appengine/findit/waterf...
appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:109:
master_name, builder_name, build_number - 1)
On 2016/12/14 23:33:37, stgao (slow on Monday) wrote:
> Should we check whether build_number is 0 before -1 here?

Done.

[commit-bot: I haz the power, 2016-12-15 06:38:28]

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-12-15 06:51:58]

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

[commit-bot: I haz the power, 2016-12-15 06:51:59]

Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/331a0c32909f0e10)

[stgao, 2016-12-15 22:11:06]

The CQ bit was checked by stgao@chromium.org

[commit-bot: I haz the power, 2016-12-15 22:11:14]

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-12-15 22:18:31]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481839866888100,
"parent_rev": "e0081b8776a8639ad4fc0e1d02a6714973d9f73e", "commit_rev":
"e2172016df4fce6ce4963d978e36a9bca814b402"}

[commit-bot: I haz the power, 2016-12-15 22:18:34]

Description was changed from

==========
[Findit] Flake Checker: Extract commit position and git hash and display to UI
for each analyzed build

Each analyzed build's commit position and git hash are now extracted, with links
to the commits available in the mouse-over UI in the graph.

Sample UI:
https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/check-flake?master_...

BUG=672338
==========

to

==========
[Findit] Flake Checker: Extract commit position and git hash and display to UI
for each analyzed build

Each analyzed build's commit position and git hash are now extracted, with links
to the commits available in the mouse-over UI in the graph.

Sample UI:
https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/check-flake?master_...

BUG=672338

Review-Url: https://codereview.chromium.org/2563383002
Committed:
https://chromium.googlesource.com/infra/infra/+/e2172016df4fce6ce4963d978e36a...
==========

[commit-bot: I haz the power, 2016-12-15 22:18:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/e2172016df4fce6ce4963d978e36a...