Add `metric count` evolution plot; fixes #312. #335

adrian17 · 2017-10-01T21:01:58Z

Fixes #312.

Before: link
After: link

Changes:

the new plot is displayed under the the "submissions" plot.
"submissions" option was removed from the aggregates multiselect. I didn't feel it's needed, since the submissions plot is always shown anyway
a lot of refactors, just to make the actual feature implementation commit just 20 lines long

To consider:

I'm 90% sure I got the "metric count" semantics right (it's equivalent to the Sample Count value in Distribution View), but it'd be nice if you made sure I did it right (see side question below)
The new plot is almost identical to the submissions plot (with the exception of order of magnitude of values), which makes them look a bit weird. There may be a smarter way to draw these two, but I could figure out a good (and easy to implement) UX for that.
is refactoring/reformatting/simplifying related code in feature PRs (see commit d634ade) like this OK, or is it too much code churn?

Side question: sometimes/often metric count < ping count. For example distribution, evolution. I don't get why exactly this can happen. Does the server report all pings in the time range, even if they didn't have this value? (which also seems a bit strange, since 1. that would mean the ping count should be equal between metrics, but it isn't, and 2. being default browser is probably checked fairly early on?)

chutten · 2017-10-02T13:25:50Z

The live link looks great!

Before I get into the code: generally big refactors are less useful that they appear. There's a lot to be gained by treating every unfamiliar line of code as the fix to a bug we didn't even see. That being said, these dashboards were coded rapidly, so there's probably some easy wins a refactor could provide :)

To your side question: "ping count" is only counting pings that contain the probe. "sample count" is the number of measurements. So two pings with histogram A, one with one measurement of '1' and the other with two measurements, of '1' and '7' would have a "ping count" of 2, a "sample count" of 3, and a "sample sum" of 9.

Those links you provide are likely a bug. Thanks for catching them! Thanks to your report @fbertsch is now looking into it.

chutten · 2017-10-02T13:36:43Z

new-pipeline/src/evo.js

-    } : function (y) {
-      return y;
-    },
+    yax_format: (y => y + valueSuffux),


This coerces y to a string which, for counts like submissions and metric counts, means the formatter can't automatically translate the y-ticks from, say, 150000 to 150k.

chutten · 2017-10-02T13:37:19Z

new-pipeline/src/evo.js

@@ -703,6 +703,8 @@ function getHistogramEvolutionLines(channel, version, measure, aggregates,
 }

 function displayEvolution(target, lines, usePercentages, plotOptions) {
+  let valueSuffux = usePercentages ? "%" : "";


s/Suffux/Suffix/g

chutten · 2017-10-02T13:39:18Z

new-pipeline/src/evo.js

@@ -825,11 +814,9 @@ function displayEvolution(target, lines, usePercentages, plotOptions) {
      var x = parseInt(rolloverCircle.getAttribute("cx")) + 20,
        y = 40;
      var bbox = legend[0][0].getBBox();
-      if (x + bbox.width + 50 > $(`${target} svg`)
-        .width()) x -= bbox.width + 40;
+      if (x + bbox.width + 50 > $(`${target} svg`).width()) x -= bbox.width + 40;


The style we're aspiring to use recommends three-line conditionals

if (cond) { ...body... }

chutten · 2017-10-02T13:41:49Z

new-pipeline/src/evo.js

      d3.select(`${target} .mg-active-datapoint-container`)
-        .attr("transform", "translate(" + (x + bbox.width) + "," + (y +
-          15) + ")");
+        .attr("transform", "translate(" + (x + bbox.width) + "," + (y + 15) + ")");


More concise as a template string?
translate(${x + bbox.width}, ${y + 15})

chutten · 2017-10-02T13:44:01Z

new-pipeline/src/evo.js

@@ -25,10 +24,14 @@ var gDefaultAggregates = [
  ["95th-percentile", "95th percentile", function (evolution) {
    return evolution.percentiles(95);
  }],
+];
+// these will not appear in the multiselect


I'm not sure if anyone uses it, but being able to have the submissions on the same plot instead of just on the linked plot below might support use cases requiring a more closer scrutiny of submissions vs values. For now I recommend leaving it in the multiselect.

chutten · 2017-10-02T13:54:10Z

new-pipeline/src/evo.js

  indicate("Rendering evolutions...");

+  var submissionLines = lines.filter(line => line.aggregate == "Submissions");


I'm not the biggest fan of magic strings. Can we at least pull this out to a constant? Or if you can think of something more clever, I'd like that too :)

chutten · 2017-10-02T13:55:28Z

new-pipeline/src/evo.js

+  var submissionLines = lines.filter(line => line.aggregate == "Submissions");
+
+  let metaAggregateNames = gMetaAggregates.map(entry => entry[1]);
+  lines = lines.filter(line => !metaAggregateNames.includes(line.aggregate));


This imposes a constraint that aggregates and metaAggregates cannot share the same name. Not likely to come up, but you should put it in a comment up near gMetaAggregates.

chutten · 2017-10-02T13:57:17Z

new-pipeline/src/evo.js

+  displayEvolution('#metric-count', countLines, false, {
+    height: 300,
+    x_label: variableLabel,
+    y_label: "Daily Metric Coun",


missing a "t"

chutten · 2017-10-02T13:59:18Z

v2/telemetry.js

@@ -312,6 +312,10 @@
      });
    };

+    Evolution.prototype.metric_count = function () {


telemetry.js uses camelCase for its API names.

Speaking of which, if you're adding to the API you need to add to the documentation. telemetry.js is used in a few places by a few teams and individuals. Keeping the documentation up-to-date is of utmost importance.

chutten · 2017-10-02T14:51:31Z

We've found out the problem that was showing you ping count > sample count. It's an obscure Telemetry client bug that you need not concern yourself about the details... the closest tl;dr I can give is: we were sending blank histograms for every ping after the first one for that probe (and others like it) which resulted in higher ping counts than sample counts.

The bug was noticed and fixed as a part of a large refactor in the middle of Nightly 56 which is why data from 57 and 58 do not exhibit this character.

Thanks again for catching it!

adrian17 · 2017-10-02T20:08:24Z

Pushed updated commits.

After discussion on IRC, dropped this comment.

Renamed "metric count" to "sample count", as "sample" seemed to already be used in the UI and docs.
(Another note: I called the new Evolution API function sampleCounts(). While the naming is inconsistent with submissions(), I think it's a lesser evil - .submits() would be a misleading name.)

chutten · 2017-10-02T20:29:15Z

Is the new code being served properly at the "After" link? I keep refreshing and the lower graphs keep having y-axis ticks that are full of 0s instead of being automagically shortened to things like 150k...

Also hide submissionLines from the multiselect, as it's always shown on the bottom

Also added sampleCount() to Evolution.

adrian17 · 2017-10-02T21:40:04Z

Oh, turns out the old code had a small bug too.

This coerces y to a string which, for counts like submissions and metric counts, means the formatter can't automatically translate the y-ticks from, say, 150000 to 150k.

Turns out even just y => y disables pretty formatting in MetricsGraphics.

Fixed it here.
This also enables value formatting for the main plot - see before after.

chutten · 2017-10-03T18:09:41Z

Looks good to me! Let's get this in production. (It should show up within the next half hour. Possibly faster)

Refactor duplicated evo.js plot generation into separate function

c8d456d

chutten reviewed Oct 2, 2017

View reviewed changes

adrian17 force-pushed the gh-pages branch from bd190aa to ec3ba9e Compare October 2, 2017 20:02

adrian17 added 3 commits October 2, 2017 23:34

Minor code cleanup/modernisation in displayEvolution

b8e3ba3

Refactor plot generation to not pass submissionLines everywhere

db90552

Also hide submissionLines from the multiselect, as it's always shown on the bottom

Add a separate Sample Count plot at the bottom.

a7123b2

Also added sampleCount() to Evolution.

adrian17 force-pushed the gh-pages branch from ec3ba9e to a7123b2 Compare October 2, 2017 21:35

chutten approved these changes Oct 3, 2017

View reviewed changes

chutten merged commit d6a9871 into mozilla:gh-pages Oct 3, 2017

chutten mentioned this pull request Oct 24, 2017

Cerberus hasn't run since Oct 12 mozilla/cerberus#33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `metric count` evolution plot; fixes #312. #335

Add `metric count` evolution plot; fixes #312. #335

adrian17 commented Oct 1, 2017 •

edited

Loading

chutten commented Oct 2, 2017

chutten Oct 2, 2017

chutten Oct 2, 2017

chutten Oct 2, 2017

chutten Oct 2, 2017

chutten Oct 2, 2017

chutten Oct 2, 2017

chutten Oct 2, 2017

chutten Oct 2, 2017

chutten Oct 2, 2017

chutten commented Oct 2, 2017

adrian17 commented Oct 2, 2017

chutten commented Oct 2, 2017

adrian17 commented Oct 2, 2017

chutten commented Oct 3, 2017

		indicate("Rendering evolutions...");

		var submissionLines = lines.filter(line => line.aggregate == "Submissions");

Add metric count evolution plot; fixes #312. #335

Add metric count evolution plot; fixes #312. #335

Conversation

adrian17 commented Oct 1, 2017 • edited Loading

chutten commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chutten commented Oct 2, 2017

adrian17 commented Oct 2, 2017

chutten commented Oct 2, 2017

adrian17 commented Oct 2, 2017

chutten commented Oct 3, 2017

Add `metric count` evolution plot; fixes #312. #335

Add `metric count` evolution plot; fixes #312. #335

adrian17 commented Oct 1, 2017 •

edited

Loading