Add a central module to handle the metric data #34

georgf · 2018-07-25T09:33:01Z

Having more conversations on the usage of Redux, it does the job of centralizing the data state but might be more complex than what we need.

As an alternative to the Redux migration #26 i'm testing out moving the metric data handling into a separate module, MetricData / metricdata.js.

This centrally handles:

mapping the metric/channel/version selection to a data request
async fetching of metric data
getting summary values out of it

From there we can:

hold an instance of MetricData in App
pass around this data store in props instead of always passing many individual props
keep metric data handling logic in a central place

- to avoid case-sensitivity issues, change paths to be all lower-case - also use .jsx for components for automatic syntax highlighting in editors

React likes things to be named index.js apparently, let’s go with it.

- to avoid case-sensitivity issues, change paths to be all lower-case - also use .jsx for components for automatic syntax highlighting in editors

React likes things to be named index.js apparently, let’s go with it.

# Conflicts: # src/app.jsx

Also save some repetition from re-using variables.

georgf · 2018-07-25T09:37:51Z

John, does this approach make sense to you?

georgf · 2018-07-27T16:36:45Z

Or to you Davor?

openjck · 2018-07-27T22:06:55Z

src/app.jsx

+        prevState.activeVersion !== this.state.activeVersion) {
+      this.dataStore.loadDataFor(this.state.activeMetric, this.state.activeChannel, this.state.activeVersion)
+          .then(() => this.setState({activeData: this.dataStore.active.data}));
+    }


Memoization might make this section a little cleaner:

https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization

This is interesting, thanks.
I may have to digest that a bit and look into it in a follow-up.

openjck · 2018-07-27T22:12:30Z

I can't say I'm an expert at state management in React, but this seems fine to me. It's pretty straightforward. @spasovski any other feedback?

spasovski · 2018-07-28T00:36:48Z

src/components/summaryview.jsx

+
+    let metric = this.props.dataStore.active.metric;
+    let channel = this.props.dataStore.active.channel;
+    let version = this.props.activeVersion;


nit: these look like they cold be const

spasovski · 2018-07-28T00:50:59Z

src/metricdata.js

+    let bucketHits = 0;
+    let linearTerm = (buckets[buckets.length - 1] - buckets[buckets.length -2]) / 2;
+    let exponentialFactor = Math.sqrt(buckets[buckets.length - 1] / buckets[buckets.length - 2]);
+    let useLinearBuckets = this.kind === "linear" || this.kind === "flag" || this.kind === "boolean" || this.kind === "enumerated";


could also do let useLinearBuckets = ['linear', 'flag', 'boolean', 'enumerated'].includes(this.kind);

spasovski · 2018-07-28T00:53:15Z

src/metricdata.js

+  }
+
+  loadDataFor(metric, channel, version) {
+    return fetch("https://mozilla.github.io/mdv2/data/" +


the fetch url could be "https://mozilla.github.io/mdv2/data/${metric}_${channel}_${version}.json" with string interpolation

spasovski · 2018-07-28T00:55:21Z

src/metricdata.js

+    ];
+  }
+
+  get versionOptions() {


I haven't seen this get syntax before. Very java-ey

spasovski · 2018-07-28T00:59:47Z

Am I wrong in thinking Redux wasn't used here? Not trying to vote for or against...that's a slightly more involved conversation. John's momoization link is good reading and some of the components could be pure.

I don't have greater insight into this project but the logic itself seems fine and I like the trimming of the previously massive component into something that gets its state externally.

georgf · 2018-07-30T15:04:06Z

Thanks John & Davor, looking into it.
Flagging Chris for feedback from the project side.

georgf · 2018-07-30T16:55:19Z

Am I wrong in thinking Redux wasn't used here? Not trying to vote for or against...that's a slightly more involved conversation.

This is correct. I heard some light feedback that Redux would solve the problem at hand (centralizing data handling) but is probably more complex than required. Thus i'm testing this out as an alternative, more straight-forward solution.

- use more `const` - more readable template string - more readable metric kind comparison

chutten · 2018-08-02T20:29:04Z

src/metricdata.js

+      nfifthPercentile: "",
+      median: "",
+      mean: "",
+      lastMedian: 315,


That's oddly specific

This is copied code, let's get to that later.

chutten · 2018-08-02T20:33:14Z

src/metricdata.js

+      metric: this._active.metric,
+      channel: this._active.channel,
+      version: this._active.version,
+      data: this._active.data,


While we're having fun with ES6, this can be rewritten as return {...this._active};

chutten · 2018-08-02T20:37:55Z

src/metricdata.js

+  }
+
+  updateCachedData() {
+    this.cached.mean = this.getMean().toFixed(2);


if this.mean returns full-precision numbers, then I think this.cached.mean should be full-precision as well.

chutten · 2018-08-02T20:38:32Z

src/metricdata.js

+  }
+
+  async loadDataFor(metric, channel, version) {
+    const response = await fetch(`https://mozilla.github.io/mdv2/data/${metric}_${channel}_${version}.json`);


A relative URL could make testing easier

Let's get to that when the need comes up.

chutten · 2018-08-02T20:44:20Z

src/metricdata.js

+    this._active.metric = metric;
+    this._active.channel = channel;
+    this._active.version = version;
+    this._active.data = data;


More ES6 wizardry: this._active = {...this._active, metric, channel, version, data};

chutten · 2018-08-02T20:45:03Z

src/metricdata.js

+    return this.getChange();
+  }
+
+  updateCachedData() {


We aren't caching data, though, we're caching aggregates?

It's still cached data though?
This caching we may want to revisit soon, currently this is mostly copied over and adopted to make work.

chutten · 2018-08-02T20:46:43Z

src/metricdata.js

+  }
+
+  getMean() {
+    let currentData = this._active.data;


I'm a proponent of making const whatever we can make const. So here that'd be currentData, buckets, values, linearTerm, exponentialFactor, useLinearBuckets, centralX or so.

Ohhh, wait this is copied from erin's impl elsewhere. Nevermind, then.

chutten · 2018-08-02T20:48:11Z

src/metricdata.js

+      - buckets[buckets.length -2];
+    }*/
+
+    return  buckets[buckets.length - 1] * buckets[buckets.length - 1] / buckets[buckets.length - 2];


there's two spaces after return here

chutten · 2018-08-02T20:49:13Z

src/metricdata.js

+
+  getChange() {
+    let rawChange = this.getPercentile(50) - this.cached.lastMedian;
+    let pctChange = (rawChange / this.cached.lastMedian) * 100;


this.cached.lastMedian is never written to? Oh, I guess that's a TODO.

Yes, the lastMedian approach needs revisiting anyway (currently it doesn't show it from the preceding version, but from the last-loaded version).

georgf · 2018-08-06T14:27:43Z

@chutten Please take a look over it. Let's aim for identifying improvements as follow-ups where possible.

One follow-up thing (existing issue) i've identified is that switching to scalars_devtools_onboarding_is_devtools_user results (in the summary view) in the median value of infinity.
This seems to come from getLastBucketUpper() doing a div by 0 as buckets[buckets.length - 2] is 0.
This is not a new issue, it just is visible now because we're actively loading different data files now (we didn't before).

chutten

I leave you to decide which comments are appropriate to address now and which as follow-ups (and which to ignore and evaluate later as part of a generic cleanup sweep).

getLastBucketUpper has commented out code that requires information about what the type of measurement is. We don't have that information, but scalars_devtools_onboarding_is_devtools_user is a metric that requires the special handling. I'm not worried about that at this state as it will require broader changes to make correct.

In general I'm happy with the move and the reorganization, so in my opinion it is merge-able.

georgf · 2018-08-08T12:01:06Z

Ok, moving forward with the merge.
Let's plan a review of the code base to identify what we want to improve.

georgf added 15 commits July 19, 2018 19:16

Change paths to all lower-case & .jsx

546dcb7

- to avoid case-sensitivity issues, change paths to be all lower-case - also use .jsx for components for automatic syntax highlighting in editors

Remove duplicated index.jsx

9d3a76a

React likes things to be named index.js apparently, let’s go with it.

Fix casing in app.test.jsx

e73c27d

Factor metric data handling out of App component

9db899f

Change how we track active state in App component

20cca47

Wire up loading of data on metric changes

3ad1ffa

Change paths to all lower-case & .jsx

edb134e

- to avoid case-sensitivity issues, change paths to be all lower-case - also use .jsx for components for automatic syntax highlighting in editors

Remove duplicated index.jsx

3a1aaff

React likes things to be named index.js apparently, let’s go with it.

Factor metric data handling out of App component

a7f6d43

Change how we track active state in App component

1bd843c

Wire up loading of data on metric changes

91ecb4d

Fix eslint errors

8e375de

Merge remote-tracking branch 'origin/data-store' into data-store

9e766a0

# Conflicts: # src/app.jsx

Round down summary view values to 2 digits

7932f80

Also save some repetition from re-using variables.

Fix mean display in distribution view

78c47d1

georgf requested a review from openjck July 25, 2018 09:37

georgf requested a review from spasovski July 27, 2018 16:36

openjck reviewed Jul 27, 2018

View reviewed changes

spasovski reviewed Jul 28, 2018

View reviewed changes

src/metricdata.js

];

}

get versionOptions() {

Copy link

spasovski Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen this get syntax before. Very java-ey

georgf requested a review from chutten July 30, 2018 15:03

Address code style feedback

8e74746

- use more `const` - more readable template string - more readable metric kind comparison

chutten reviewed Aug 2, 2018

View reviewed changes

Small improvement

e059463

georgf added 3 commits August 6, 2018 16:30

Use relative URL for data requests

4b14568

Bucket generation readability improvements

ca4b80a

Fix

07b4b48

chutten approved these changes Aug 7, 2018

View reviewed changes

georgf merged commit d0dc82e into mozilla:master Aug 8, 2018

This was referenced Aug 8, 2018

Attempting Redux migration #26

Closed

Aggregate statistics broken for scalars #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a central module to handle the metric data #34

Add a central module to handle the metric data #34

georgf commented Jul 25, 2018 •

edited

georgf commented Jul 25, 2018

georgf commented Jul 27, 2018

openjck Jul 27, 2018

georgf Jul 30, 2018

openjck commented Jul 27, 2018

spasovski Jul 28, 2018

spasovski Jul 28, 2018

spasovski Jul 28, 2018 •

edited

spasovski Jul 28, 2018

spasovski commented Jul 28, 2018

georgf commented Jul 30, 2018

georgf commented Jul 30, 2018

chutten Aug 2, 2018

georgf Aug 3, 2018

chutten Aug 2, 2018

chutten Aug 2, 2018

chutten Aug 2, 2018

georgf Aug 3, 2018

chutten Aug 2, 2018

chutten Aug 2, 2018

georgf Aug 3, 2018 •

edited

chutten Aug 2, 2018

chutten Aug 2, 2018

chutten Aug 2, 2018

chutten Aug 2, 2018

georgf Aug 3, 2018

georgf commented Aug 6, 2018

chutten left a comment

georgf commented Aug 8, 2018

Add a central module to handle the metric data #34

Add a central module to handle the metric data #34

Conversation

georgf commented Jul 25, 2018 • edited

georgf commented Jul 25, 2018

georgf commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjck commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spasovski Jul 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spasovski commented Jul 28, 2018

georgf commented Jul 30, 2018

georgf commented Jul 30, 2018

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

georgf Aug 3, 2018 • edited

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

georgf commented Aug 6, 2018

chutten left a comment

Choose a reason for hiding this comment

georgf commented Aug 8, 2018

georgf commented Jul 25, 2018 •

edited

spasovski Jul 28, 2018 •

edited

georgf Aug 3, 2018 •

edited