[Uptime] Improve monitor charts query #30561

justinkambic · 2019-02-08T19:52:14Z

Summary

Resolves #29843.

This change would update the query used for fetching monitor chart data, and clean up computations done on the client that probably belong in the server.

Testing this PR

The changes enacted will have visual results, so aside from providing code review you should be able to see it's working as intended.

Start up Heartbeat
Select a monitor
Verify that the duration value in the status bar looks correct. Things to watch for:
- Make sure the value isn't really fast. We had a bug recently that caused the duration value to be "converted" twice, meaning val / (1000 * 1000). A request to https://www.elastic.co is not going to complete in 0.15ms.
- Likewise, make sure the value doesn't look too large, i.e. not being converted at all. A request to https://www.elastic.co should not take 150,000ms either.
Verify the duration stats in the duration chart seem like they're on an appropriate scale. I've included a sample screenshot below:

~~This PR is in progress, and shouldn't be reviewed until #30441 is merged.~~

elasticmachine · 2019-02-08T19:52:16Z

Pinging @elastic/uptime

elasticmachine · 2019-02-08T21:46:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

andrewvc · 2019-02-08T22:45:15Z

I know this is in progress, but I'd like to add a piece of feedback. The query changes are great.

Is there a concrete need for the reformatting to be moved to the server? The chart takes a weird format. That responsibility seems more tightly coupled to the Eui widget than anything else. From a performance standpoint where that transformation happens seems negligible.

If we do that formatting on the server that doesn't have much of an impact today due to the GQL API being private. However, if we decide to create a public GQL API in the future that makes things awkward. Generally speaking APIs are cleaner if they are more agnostic of consumer details.

elasticmachine · 2019-02-11T16:42:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

justinkambic · 2019-02-11T16:52:11Z

If we do that formatting on the server that doesn't have much of an impact today due to the GQL API being private. However, if we decide to create a public GQL API in the future that makes things awkward. Generally speaking APIs are cleaner if they are more agnostic of consumer details.

Yeah that makes sense, we should remove that bit from this change.

…ema naming.

elasticmachine · 2019-02-12T15:35:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

justinkambic · 2019-02-12T16:24:40Z

@andrewvc if you check out 7b0bbe4, it should address your concerns about unit conversion.

justinkambic · 2019-02-12T16:59:29Z

x-pack/plugins/uptime/public/components/functional/monitor_status_bar.tsx

-          description="The 'ms' is an abbreviation for 'milliseconds'."
-        />
-      </EuiFlexItem>
+      {!!duration && (


I think we're ok to just omit this element if there's no duration for some reason. There should always be one though.

elasticmachine · 2019-02-13T22:21:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-14T16:48:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

andrewvc

Functionality is great, but I think the code could use some tweaks. See included comments :)

andrewvc · 2019-02-14T20:04:19Z

x-pack/plugins/uptime/server/graphql/monitors/schema.gql.ts

+    "The timeseries value for this point in time."
+    x: UnsignedInteger!
+    "The min duration value in microseconds at this time."
+    y0: Float


I realllly dislike the y0, y distinction, but I'm OK with it here since this is a private API.

If we ever make it public I'd like it to be yMin,yMax or similar. I'll leave it to you whether you feel it should be changed for this PR.

Oh ok - I don't have any objection to changing the name. I don't like that convention either.

@andrewvc if you take a look at e4ea5b5, let me know what you think.

andrewvc · 2019-02-14T20:06:22Z

x-pack/plugins/uptime/server/graphql/monitors/schema.gql.ts

+  }
+
+  "Represents the average monitor duration ms at a point in time."
+  type MonitorDurationAveragePoint {


I feel like at some later point we'll have more generic CartesianPoint types, but I'm fine with this for now.

I tend to think of these chart-related bits of code as in a state of flux that will be overwritten/deleted (because we're going to need to re-implement the charts when new EUI components are available). We will want to take care that we approach it with nuance and sensibility when we overwrite what we have today.

andrewvc · 2019-02-14T20:16:24Z

x-pack/plugins/uptime/server/lib/adapters/monitors/elasticsearch_monitors_adapter.ts

+    const mappedBuckets = buckets.map(bucket => {
+      const x = get(bucket, 'key');
+      const docCount = get(bucket, 'doc_count', 0);
+      statusMaxCount = Math.max(docCount, statusMaxCount);


This needs comments for clarity, it's pretty confusing what's going on here. A comment saying "This can be used to calculate the maximum Y bounds for a chart" would be quite helpful.

I added some comments in 3ccdd1d, please elaborate if you still think it's unclear.

andrewvc · 2019-02-14T20:34:00Z

x-pack/plugins/uptime/server/lib/adapters/monitors/elasticsearch_monitors_adapter.ts

+      durationMaxValue,
+      statusMaxCount,
+    };
+    return mappedBuckets.reduce(


Is map/reduce getting us much here?

As a fan of FP (and former lisper) I like MR, but it seems to be interfering with code clarity here. We have an entire intermediate set of variables created in the map phase that only exists to be discarded and renamed in the reduce phase.

This would be more clear to read using a simple forEach that mutated the counter object. Something like the untested code below:

const result: MonitorChart = { durationArea: [], durationLine: [], status: [], durationMaxValue: 0, statusMaxCount: 0, }; buckets.forEach(bucket => { const x = get(bucket, 'key'); const docCount = get(bucket, 'doc_count', 0); result.statusMaxCount = Math.max(docCount, result.statusMaxCount); result.durationMaxValue = Math.max(result.durationMaxValue, get(bucket, 'duration.max', 0)); result.durationArea.push({ x, y0: get(bucket, 'duration.min', null), y: get(bucket, 'duration.max', null)}); result.durationLine.push({ x, y: get(bucket, 'duration.avg', null) }); result.status.push(formatStatusBuckets(x, get(bucket, 'status.buckets', []), docCount)); }); return result;

Alternatively, if you did want to use map reduce, I'd skip the map phase completely. The goal here is to turn a list (of buckets) into a single value. That corresponds to a reduce. The code as it stands uses a map phase to expose an intermediate state. However, that intermediate state is, I would argue, confusing, because it doesn't do much other than rename variables. You could do something similar to the forEach above, but use reduce instead. Like (also untested):

const result: MonitorChart = { durationArea: [], durationLine: [], status: [], durationMaxValue: 0, statusMaxCount: 0, }; return buckets.reduce( (bucket, acc) => { const x = get(bucket, 'key'); const docCount = get(bucket, 'doc_count', 0); result.statusMaxCount = Math.max(docCount, result.statusMaxCount); result.durationMaxValue = Math.max(result.durationMaxValue, get(bucket, 'duration.max', 0)); result.durationArea.push({ x, y0: get(bucket, 'duration.min', null), y: get(bucket, 'duration.max', null)}); result.durationLine.push({ x, y: get(bucket, 'duration.avg', null) }); result.status.push(formatStatusBuckets(x, get(bucket, 'status.buckets', []), docCount)); return result; }, result);

First of all - thank you for taking the time to write all this out.

Secondly I agree with your points. Sometimes we are focused on the code's output so much we forget that others will need to read it in the future, and we implement the very first solution that comes to mind. I think your first solution is best. If we need to declare an accumulator variable before the functions no matter what, it makes sense to access it from the simplest possible block, which in this case would be forEach.

Please review 3ccdd1d when you have a chance.

elasticmachine · 2019-02-15T17:57:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…tional component and add unit test.

elasticmachine · 2019-02-16T00:25:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

andrewvc

LGTM. Looking good!

justinkambic · 2019-03-01T16:39:00Z

jenkins test this

justinkambic · 2019-03-01T16:39:33Z

@andrewvc I'm going to re-test just because these changes were last built two weeks ago. If CI passes we'll be good to merge IMO.

elasticmachine · 2019-03-01T17:35:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-01T20:21:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Refactor chart querying. * Fix monitor chart query. * Refactor several inline computations to helper functions. Improve schema naming. * Move unit conversion to client, remove bare conversion values. * Add API tests for monitor charts. * Add test for conversion function. * Add type annotations to latest schema additions. * Fix typo. * Refactor based on PR feedback, add comments asked for in PR feedback. * Rename fields in schema, update tests. Extract monitor charts to functional component and add unit test.

justinkambic · 2019-03-05T19:45:23Z

Backported to:

7.x/7.1.0 f5890e7
[Uptime] [Backport] [7.x] Improve monitor charts query (#30561) #32344
7.0.0 507e713
[Uptime] [Backport] [7.0] Improve monitor charts query (#30561) #32343

justinkambic added bug Fixes for quality problems that affect the customer experience WIP Work in progress v7.0.0 refactoring Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v6.7.0 labels Feb 8, 2019

justinkambic self-assigned this Feb 8, 2019

justinkambic added 3 commits February 12, 2019 09:38

Refactor chart querying.

3dd0657

Fix monitor chart query.

0618882

Refactor several inline computations to helper functions. Improve sch…

6eea265

…ema naming.

justinkambic force-pushed the uptime_improve-monitor-charts-query branch from 37faba6 to 6eea265 Compare February 12, 2019 14:41

Move unit conversion to client, remove bare conversion values.

7b0bbe4

Add API tests for monitor charts.

a621cab

justinkambic added review and removed WIP Work in progress labels Feb 12, 2019

justinkambic commented Feb 12, 2019

View reviewed changes

justinkambic added 4 commits February 12, 2019 12:03

Add test for conversion function.

594d8ad

Add type annotations to latest schema additions.

8bf5365

Fix typo.

6624c19

Merge branch 'master' into uptime_improve-monitor-charts-query

6a15f1e

Merge branch 'master' into uptime_improve-monitor-charts-query

c680c69

justinkambic requested a review from andrewvc February 14, 2019 15:51

andrewvc requested changes Feb 14, 2019

View reviewed changes

Refactor based on PR feedback, add comments asked for in PR feedback.

3ccdd1d

Rename fields in schema, update tests. Extract monitor charts to func…

e4ea5b5

…tional component and add unit test.

andrewvc approved these changes Mar 1, 2019

View reviewed changes

Merge branch 'master' into uptime_improve-monitor-charts-query

33cfda1

justinkambic merged commit 729fdfd into elastic:master Mar 1, 2019

justinkambic added backport pending v8.0.0 v7.2.0 labels Mar 1, 2019

This was referenced Mar 1, 2019

[Uptime] [Backport] [7.0] Improve monitor charts query (#30561) #32343

Merged

[Uptime] [Backport] [7.x] Improve monitor charts query (#30561) #32344

Merged

justinkambic deleted the uptime_improve-monitor-charts-query branch March 5, 2019 19:45

justinkambic removed backport pending v6.7.0 labels Mar 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Uptime] Improve monitor charts query #30561

[Uptime] Improve monitor charts query #30561

justinkambic commented Feb 8, 2019 •

edited

Loading

elasticmachine commented Feb 8, 2019

elasticmachine commented Feb 8, 2019

andrewvc commented Feb 8, 2019

elasticmachine commented Feb 11, 2019

justinkambic commented Feb 11, 2019

elasticmachine commented Feb 12, 2019

justinkambic commented Feb 12, 2019

justinkambic Feb 12, 2019

andrewvc Feb 14, 2019

elasticmachine commented Feb 13, 2019

elasticmachine commented Feb 14, 2019

andrewvc left a comment

andrewvc Feb 14, 2019

justinkambic Feb 15, 2019

justinkambic Feb 15, 2019

andrewvc Feb 14, 2019

justinkambic Feb 15, 2019

andrewvc Feb 14, 2019

justinkambic Feb 15, 2019

andrewvc Feb 14, 2019

justinkambic Feb 15, 2019

elasticmachine commented Feb 15, 2019

elasticmachine commented Feb 16, 2019

andrewvc left a comment

justinkambic commented Mar 1, 2019

justinkambic commented Mar 1, 2019

elasticmachine commented Mar 1, 2019

elasticmachine commented Mar 1, 2019

justinkambic commented Mar 5, 2019

[Uptime] Improve monitor charts query #30561

[Uptime] Improve monitor charts query #30561

Conversation

justinkambic commented Feb 8, 2019 • edited Loading

Summary

Testing this PR

elasticmachine commented Feb 8, 2019

elasticmachine commented Feb 8, 2019

💔 Build Failed

andrewvc commented Feb 8, 2019

elasticmachine commented Feb 11, 2019

💚 Build Succeeded

justinkambic commented Feb 11, 2019

elasticmachine commented Feb 12, 2019

💚 Build Succeeded

justinkambic commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 13, 2019

💔 Build Failed

elasticmachine commented Feb 14, 2019

💚 Build Succeeded

andrewvc left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 15, 2019

💚 Build Succeeded

elasticmachine commented Feb 16, 2019

💚 Build Succeeded

andrewvc left a comment

Choose a reason for hiding this comment

justinkambic commented Mar 1, 2019

justinkambic commented Mar 1, 2019

elasticmachine commented Mar 1, 2019

💔 Build Failed

elasticmachine commented Mar 1, 2019

💚 Build Succeeded

justinkambic commented Mar 5, 2019

justinkambic commented Feb 8, 2019 •

edited

Loading