Adding in abs delta column to the alerts table

Adding in an absolute delta column to the alerts table. 
Uses tracing's unit formatter.

http://i.imgur.com/Diz0Tix.png

BUG=catapult:#2721

Review-Url: https://codereview.chromium.org/2557743004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/041b58513c9679b8d609c6e67f50e37ed7e893d3

[jessimb, 2016-12-07 21:04:16]

The CQ bit was checked by jessimb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 21:04:27]

Dry run: 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-07 21:07:16]

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

[commit-bot: I haz the power, 2016-12-07 21:07:17]

Dry run: Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[jessimb, 2016-12-07 21:51:22]

The CQ bit was checked by jessimb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 21:51:24]

Dry run: 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-07 21:54:14]

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

[commit-bot: I haz the power, 2016-12-07 21:54:14]

Dry run: Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[jessimb, 2016-12-07 22:02:12]

The CQ bit was checked by jessimb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:02:19]

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

[jessimb, 2016-12-07 22:06:08]

Description was changed from

==========
Adding in abs delta column to alerts table

BUG=catapult:#2721
==========

to

==========
Adding in an absolute delta column to alerts table. 
Uses tracing's unit formatter.

http://i.imgur.com/kMCWPk2.png

BUG=catapult:#2721
==========

[jessimb, 2016-12-07 22:06:41]

Description was changed from

==========
Adding in an absolute delta column to alerts table. 
Uses tracing's unit formatter.

http://i.imgur.com/kMCWPk2.png

BUG=catapult:#2721
==========

to

==========
Adding in an absolute delta column to the alerts table. 
Uses tracing's unit formatter.

http://i.imgur.com/kMCWPk2.png

BUG=catapult:#2721
==========

[jessimb, 2016-12-07 22:20:06]

jessimb@chromium.org changed reviewers:
+ eakuefner@chromium.org

[jessimb, 2016-12-07 22:20:07]

PTAL

[jessimb, 2016-12-07 22:37:58]

The CQ bit was checked by jessimb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:38:08]

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

[eakuefner, 2016-12-07 22:47:59]

eakuefner@chromium.org changed reviewers:
+ sullivan@chromium.org

[eakuefner, 2016-12-07 22:48:00]

lgtm, but what about the colorization?

+sullivan for dashboard/ OWNERS

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/alerts-table-test.html (right):

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table-test.html:23: {
nit: please indent everything from here up until the ] by two more spaces.

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/alerts-table.html (right):

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:490: if (key == 'absolute_delta')
{
nit: please use strict equality/inequality (===) here and throughout.

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/group-report-page.html (right):

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
dashboard/dashboard/elements/group-report-page.html:145: {
and here.

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/mod...
File dashboard/dashboard/models/anomaly.py (right):

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/mod...
dashboard/dashboard/models/anomaly.py:68: """The percent change from before the
anomaly to after."""
s/percent //

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/mod...
dashboard/dashboard/models/anomaly.py:95: return str('%f' %
abs(self.absolute_delta))
You should either str() the abs or use a format string, but not both. This usage
of str looks like a no-op right now.

[commit-bot: I haz the power, 2016-12-07 23:14:31]

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

[commit-bot: I haz the power, 2016-12-07 23:14:32]

Dry run: This issue passed the CQ dry run.

[jessimb, 2016-12-08 00:03:28]

Description was changed from

==========
Adding in an absolute delta column to the alerts table. 
Uses tracing's unit formatter.

http://i.imgur.com/kMCWPk2.png

BUG=catapult:#2721
==========

to

==========
Adding in an absolute delta column to the alerts table. 
Uses tracing's unit formatter.

http://i.imgur.com/Diz0Tix.png

BUG=catapult:#2721
==========

[jessimb, 2016-12-08 00:19:13]

Response to eakuefner's code review. Added colorization as well.

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/alerts-table-test.html (right):

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table-test.html:23: {
On 2016/12/07 at 22:48:00, eakuefner wrote:
> nit: please indent everything from here up until the ] by two more spaces.

Done

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/alerts-table.html (right):

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
dashboard/dashboard/elements/alerts-table.html:490: if (key == 'absolute_delta')
{
On 2016/12/07 at 22:48:00, eakuefner wrote:
> nit: please use strict equality/inequality (===) here and throughout.

Done

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
File dashboard/dashboard/elements/group-report-page.html (right):

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/ele...
dashboard/dashboard/elements/group-report-page.html:145: {
On 2016/12/07 at 22:48:00, eakuefner wrote:
> and here.

Done

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/mod...
File dashboard/dashboard/models/anomaly.py (right):

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/mod...
dashboard/dashboard/models/anomaly.py:68: """The percent change from before the
anomaly to after."""
On 2016/12/07 at 22:48:00, eakuefner wrote:
> s/percent //

Oops. Done.

https://codereview.chromium.org/2557743004/diff/60001/dashboard/dashboard/mod...
dashboard/dashboard/models/anomaly.py:95: return str('%f' %
abs(self.absolute_delta))
On 2016/12/07 at 22:48:00, eakuefner wrote:
> You should either str() the abs or use a format string, but not both. This
usage of str looks like a no-op right now.

fixed in previous function as well

[sullivan, 2016-12-08 21:50:59]

https://codereview.chromium.org/2557743004/diff/80001/dashboard/dashboard/ale...
File dashboard/dashboard/alerts.py (right):

https://codereview.chromium.org/2557743004/diff/80001/dashboard/dashboard/ale...
dashboard/dashboard/alerts.py:167: 'units': anomaly_entity.units
So the implementation of units is that it does self.GetTestMetadataKey().get()
under the covers. This page is currently very optimized not to need to call
get() on the TestMetadata entities because that takes time (usually tens of
milliseconds but sometimes hiccups into the hundreds). And we are doing it
serially for each entity, which makes it slower.

I'd strongly prefer to store the units at alert creation time, just like we
store median_after_anomaly, etc. That means that you'll need to write a job that
updates old anomalies to have units.

[jessimb, 2016-12-14 00:12:53]

Now I use the existing unit in anomaly. If a unit does not exist, we display
"units - unformatted". See a demo here
https://dev-jessimb-032df291-dot-chromeperf.appspot.com/alerts?sortby=end_rev...

Also added testing for find_anomalies to make sure the unit is grabbed from the
test.

https://codereview.chromium.org/2557743004/diff/80001/dashboard/dashboard/ale...
File dashboard/dashboard/alerts.py (right):

https://codereview.chromium.org/2557743004/diff/80001/dashboard/dashboard/ale...
dashboard/dashboard/alerts.py:167: 'units': anomaly_entity.units
On 2016/12/08 at 21:50:58, sullivan wrote:
> So the implementation of units is that it does self.GetTestMetadataKey().get()
under the covers. This page is currently very optimized not to need to call
get() on the TestMetadata entities because that takes time (usually tens of
milliseconds but sometimes hiccups into the hundreds). And we are doing it
serially for each entity, which makes it slower.
> 
> I'd strongly prefer to store the units at alert creation time, just like we
store median_after_anomaly, etc. That means that you'll need to write a job that
updates old anomalies to have units.

Done! With that aid of https://codereview.chromium.org/2571053002 we now store
the units in the alert.

[sullivan, 2016-12-14 00:16:57]

lgtm

I am fine with landing and deploying this even if the map reduce is still going,
it should help us track how it is working.

[jessimb, 2016-12-14 01:33:46]

The CQ bit was checked by jessimb@chromium.org

[jessimb, 2016-12-14 01:33:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org
Link to the patchset: https://codereview.chromium.org/2557743004/#ps120001
(title: "adding in tests")

[commit-bot: I haz the power, 2016-12-14 01:34: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-12-14 02:04:20]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481679226642210,
"parent_rev": "b7b743b57627c97cc62700ab07c1214bd677a37b", "commit_rev":
"041b58513c9679b8d609c6e67f50e37ed7e893d3"}

[commit-bot: I haz the power, 2016-12-14 02:04:23]

Description was changed from

==========
Adding in an absolute delta column to the alerts table. 
Uses tracing's unit formatter.

http://i.imgur.com/Diz0Tix.png

BUG=catapult:#2721
==========

to

==========
Adding in an absolute delta column to the alerts table. 
Uses tracing's unit formatter.

http://i.imgur.com/Diz0Tix.png

BUG=catapult:#2721

Review-Url: https://codereview.chromium.org/2557743004
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-12-14 02:04:24]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...