708059 - Add a page for endurance tests charts to the dashboard

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Result Dashboard, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

It would be great for visibility and regression detection to have dynamically plotted charts for endurance testruns. I would suggest the following initial charts:

1. Test status (pass, skip, fail)
2. Max explicit memory
3. Max resident memory
4. Testrun duration

This would allow us to quickly identify a spike in any one of these metrics, and will mean that we can stop gathering a subset of this data manually.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add endurance tests charts. v1.0

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 579420 [details] [diff] [review]
Add endurance tests charts. v1.0

>+++ b/_attachments/js/dashboard.js
>-          value.time = new Date(value.time).toISOString();
>+          value.time = new Date(value.time_start).toISOString();

Where has this been changed. Do I miss something?

>-          memory_label = (value.stats && value.stats.explicit) ? "explicit" : "allocated";
>-          value.min_memory = (value.stats && value.stats[memory_label]) ? Math.round(value.stats[memory_label].min * BYTE_TO_MEGABYTE) : METRIC_UNAVAILABLE;
>-          value.max_memory = (value.stats && value.stats[memory_label]) ? Math.round(value.stats[memory_label].max * BYTE_TO_MEGABYTE) : METRIC_UNAVAILABLE;
>+          value.memory = value.stats ? get_memory_stats(value.stats) : {};

Lets not fold two different patches into this feature enhancement. 

>+    var endurance_charts = function() {
>+      var branch = this.params.branch ? this.params.branch : '11.0';
>+      var platform = this.params.platform ? this.params.platform : 'All';

The default for branch should be 'All'.

>+      var fromDate;
>+      if (this.params.from) {
>+        fromDate = new Date(this.params.from);
>+      }
>+      else {
>+        fromDate = new Date();
>+        fromDate.setDate(fromDate.getDate() - 28);

Do we want to go back that far? What are the performance implications? 

>@@ -1580,6 +1683,7 @@ function processTestResults(aReport) {
>     this.get('#/l10n/report/:id', l10n_report);
>     this.get('#/endurance', endurance_reports);
>     this.get('#/endurance/reports', endurance_reports);
>+    this.get('#/endurance/charts', endurance_charts);

I think that we should also make it the default view when clicking on the endurance tests link.

>+++ b/_attachments/templates/endurance_charts.mustache
>+        function convertDateToUTC(date) {
>+            return Date.UTC(date.getUTCFullYear(), date.getUTCMonth(), date.getUTCDate(), date.getUTCHours(), date.getUTCMinutes(), date.getUTCSeconds());

Why do we need this in the template? I would rather see an update to the date.format.js file, and the conversion in the dashboard.js file.

>+++ b/dashboard.js
>@@ -256,7 +256,8 @@ var enduranceReportsMap = function(doc) {
>     var application_branch = doc.application_version.match(/(\d+\.\d+)\.*/)[1];
> 
>     var r = {
>-      time : doc.time_start,
>+      time_start : doc.time_start,
>+      time_end : doc.time_end,

Can we do this on a separate bug and update all available views with it? It shouldn't only be used in this view.

Otherwise I kinda would like to see a link to a demo page if possible.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 579420 [details] [diff] [review] [diff] [details] [review]
> Add endurance tests charts. v1.0
> 
> >+++ b/_attachments/js/dashboard.js
> >-          value.time = new Date(value.time).toISOString();
> >+          value.time = new Date(value.time_start).toISOString();
> 
> Where has this been changed. Do I miss something?

See later in the patch, where you've also commented on this change.

> 
> >-          memory_label = (value.stats && value.stats.explicit) ? "explicit" : "allocated";
> >-          value.min_memory = (value.stats && value.stats[memory_label]) ? Math.round(value.stats[memory_label].min * BYTE_TO_MEGABYTE) : METRIC_UNAVAILABLE;
> >-          value.max_memory = (value.stats && value.stats[memory_label]) ? Math.round(value.stats[memory_label].max * BYTE_TO_MEGABYTE) : METRIC_UNAVAILABLE;
> >+          value.memory = value.stats ? get_memory_stats(value.stats) : {};
> 
> Lets not fold two different patches into this feature enhancement. 

This was actually changed in this patch first due to another avenue that was rejected. It's the main reason this is feedback? and not review? of course the other patch will be merged in once it lands.

> 
> >+    var endurance_charts = function() {
> >+      var branch = this.params.branch ? this.params.branch : '11.0';
> >+      var platform = this.params.platform ? this.params.platform : 'All';
> 
> The default for branch should be 'All'.

I strongly disagree. A branch should be selected otherwise the charts will be a hot mess. We may want to compare different branches charts side-by-side but I can't see value in plotting multiple branches on one chart. Ideally we'd select the latest branch be default, but I wasn't sure if we could do this.

> >+      var fromDate;
> >+      if (this.params.from) {
> >+        fromDate = new Date(this.params.from);
> >+      }
> >+      else {
> >+        fromDate = new Date();
> >+        fromDate.setDate(fromDate.getDate() - 28);
> 
> Do we want to go back that far? What are the performance implications? 

The last month of results feels the most valuable to me. I was even contemplating disabling the filter here, but that made testing difficult with the data I had.

> >@@ -1580,6 +1683,7 @@ function processTestResults(aReport) {
> >     this.get('#/l10n/report/:id', l10n_report);
> >     this.get('#/endurance', endurance_reports);
> >     this.get('#/endurance/reports', endurance_reports);
> >+    this.get('#/endurance/charts', endurance_charts);
> 
> I think that we should also make it the default view when clicking on the
> endurance tests link.

I'd like to get more thoughts on that. It would be inconsistent with other testrun types, but I do see the value.

> >+++ b/_attachments/templates/endurance_charts.mustache
> >+        function convertDateToUTC(date) {
> >+            return Date.UTC(date.getUTCFullYear(), date.getUTCMonth(), date.getUTCDate(), date.getUTCHours(), date.getUTCMinutes(), date.getUTCSeconds());
> 
> Why do we need this in the template? I would rather see an update to the
> date.format.js file, and the conversion in the dashboard.js file.

Agreed, I had intended tidying this up but missed it.

> >+++ b/dashboard.js
> >@@ -256,7 +256,8 @@ var enduranceReportsMap = function(doc) {
> >     var application_branch = doc.application_version.match(/(\d+\.\d+)\.*/)[1];
> > 
> >     var r = {
> >-      time : doc.time_start,
> >+      time_start : doc.time_start,
> >+      time_end : doc.time_end,
> 
> Can we do this on a separate bug and update all available views with it? It
> shouldn't only be used in this view.

Sounds good to me.

> Otherwise I kinda would like to see a link to a demo page if possible.

Unfortunately this isn't easy. In order to get these charts looking good I've had to replicate mozmill-release locally. I had a lot of issues trying to replicate it to my couchone instance. Would you be satisfied with an up to date screenshot from my local instance?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Dave Hunt (:davehunt) from comment #3)
> > >+++ b/_attachments/js/dashboard.js
> > >-          value.time = new Date(value.time).toISOString();
> > >+          value.time = new Date(value.time_start).toISOString();
> > 
> > Where has this been changed. Do I miss something?
> 
> See later in the patch, where you've also commented on this change.

Yeah, saw it but forgot to remove this comment in my reply.

> This was actually changed in this patch first due to another avenue that was
> rejected. It's the main reason this is feedback? and not review? of course
> the other patch will be merged in once it lands.

Ok, so if you have another update here, please make sure it is based on the other patch.

> > The default for branch should be 'All'.
> 
> I strongly disagree. A branch should be selected otherwise the charts will
> be a hot mess. We may want to compare different branches charts side-by-side
> but I can't see value in plotting multiple branches on one chart. Ideally
> we'd select the latest branch be default, but I wasn't sure if we could do
> this.

Makes sense. Even performance wise. So we should have a global const with the latest version, or even better an array with all versions so we can simplify the templates. Would you mind doing such a change in a separate bug? I would really appreciate it.

> > Do we want to go back that far? What are the performance implications? 
> 
> The last month of results feels the most valuable to me. I was even
> contemplating disabling the filter here, but that made testing difficult
> with the data I had.

What about performance? How long does it take to show up the chart page?

> > I think that we should also make it the default view when clicking on the
> > endurance tests link.
> 
> I'd like to get more thoughts on that. It would be inconsistent with other
> testrun types, but I do see the value.

We could keep reports if you want, and switch over to charts once we have those for all test-runs.

> Unfortunately this isn't easy. In order to get these charts looking good
> I've had to replicate mozmill-release locally. I had a lot of issues trying
> to replicate it to my couchone instance. Would you be satisfied with an up
> to date screenshot from my local instance?

When you have an updated patch ready, I could check it locally then. It's ok for now. Thanks Dave.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/6

Pointer to Github pull-request

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Screenshot of charts for Firefox 8 results during August.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 582313 [details]
Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/6

Let me give feedback for now until the dependency bug has been fixed and the patch is way cleaner. In general it looks kinda good to me. I have had to make only some comments, which we should be able to sort out quickly.

The most important question is why we need this special UTC date handling code. If you can explain it and come up with details and an example I would kinda appreciate it.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 582313 [details]
Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/6

Updated pull request. Dependent patch has been merged and landed, and I've implemented the suggested changes.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 582313 [details]
Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/6

Comments made on the pull request, which have to be addressed:

* Order of graphs
* Changes to get_memory_stats and overall usage

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/6/files

Pointer to Github pull-request

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/6/files#

Pointer to Github pull-request

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 583461 [details]
Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/6/files#

Implemented suggested changes from review.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 583461 [details]
Pointer to Github pull request: https://github.com/whimboo/mozmill-dashboard/pull/6/files#

Looks good. I can land it for you and push directly to production.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
https://github.com/whimboo/mozmill-dashboard/commit/412aa67665d2a85b0c428f50056c899f0b4508fa

Push to production happened. Indexer is now updating views which will take a bit.