[Uptime] Improve query performance with Heartbeat 7.6+ data. #52433

andrewvc · 2019-12-06T20:37:10Z

Summary

This PR optimizes both the snapshot component and the monitor list on the overview page by using the new monitor.timespan field from elastic/beats#14778. Note that the functionality here will work with heartbeats lacking that patch, but the performance improvements will be absent.

This PR adapts the snapshot tests to use synthetically generated data which should be easier to maintain. As a result some of that code is refactored as well.

See #52433 parent issue as well.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-12-06T20:37:12Z

Pinging @elastic/uptime (Team:uptime)

x-pack/legacy/plugins/uptime/server/lib/helper/make_date_rate_filter.ts

justinkambic

I left a few comments, only one that I think is imperative. Once we've discussed these points I think we are pretty much there. I tested locally and the functionality seems to be working great, code looks good.

justinkambic · 2020-01-07T17:59:07Z

...gacy/plugins/uptime/public/components/functional/monitor_list/monitor_list_status_column.tsx

      return i18n.translate('xpack.uptime.monitorList.statusColumn.downLabel', {
        defaultMessage: 'Down',
      });
-    case 'mixed':


nice seeing this code getting deleted

justinkambic · 2020-01-07T18:03:51Z

x-pack/legacy/plugins/uptime/public/components/functional/charts/donut_chart_legend_row.tsx

-    <EuiFlexItemAlignRight component="span">{content}</EuiFlexItemAlignRight>
+    <EuiFlexItemAlignRight
+      component="span"
+      data-test-subj={`xpack.uptime.donutChart.${message.toLowerCase()}`}


So, I've had some time to think this over and I think we may want a dedicated prop that makes the determination of what string to render here. The value that we pass to this component from its parent today isn't a constant string but rather the result of a translation call. This seems like it leaves the door open for some problematic value.

Instead we could just specify data-test-subj as an optional prop in the component's interface, and supply hardcoded string values in donut_chart_legend.tsx. WDYT?

Done in 5d789d0

I think I did a poor job explaining what I was looking for. Here're some modified snippets of what I had in mind, LMK what you think of this:

Chart Legend Row

This is where we need the attribute applied so the test utils can find our values:

interface Props { 'data-test-subj': string; ... } export const DonutChartRow = ({ 'data-test-subj': dts, ... }) => ( <EuiFlexGroup ...> <EuiFlexItemAlignRight component="span" data-test-subj={dts}> {content} </EuiFlexItemAlignRight> </EuiFlexGroup> );

Chart Legend

Then in here we could just hardcode the props as part of our component definition, and we won't be relying on the translated message strings for our attributes.

<DonutChartLegendRow color={danger} content={down} message={i18n.translate('xpack.uptime.snapshot.donutChart.legend.downRowLabel', { defaultMessage: 'Down', })} data-test-subj="xpack.uptime.snapshot.donutChart.down" /> <DonutChartLegendRow color={gray} content={up} message={i18n.translate('xpack.uptime.snapshot.donutChart.legend.upRowLabel', { defaultMessage: 'Up', })} data-test-subj="xpack.uptime.snapshot.donutChart.up" />

Doing it this way we will only be touching two files, and modifying the only components that should really care about these attributes; plus the full values are readable and searchable if someone is trying to trace these keys from the test files for some reason.

Ah, I see. My thinking is that we may want to reuse this donut chart elsewhere. By making this a prefix rather than the full subject, and passing it in from the top we could re-use the donut chart elsewhere and give it a separate data-test-subj namespace.

justinkambic · 2020-01-07T18:25:52Z

...cy/plugins/uptime/server/lib/adapters/monitor_states/elasticsearch_monitor_states_adapter.ts

+      query: { bool: { filter: await context.dateAndCustomFilters() } },
+      aggs: {
+        unique: {
+          cardinality: { field: 'monitor.id', precision_threshold: 40000 },


It might be clearer to declare a constant like MAX_SUPPORTED_PRECISION_THRESHOLD to make it clearer why we've chosen 40000 as the value for precision_threshold.

I'll add a comment here. 40k is actually the max precision the cardinality aggregation supports. We want max precision here, and the storage tradeoff is fine since we don't have a ton of buckets.

Commented added in 5d789d0#diff-d51e4dd872235464063c1c97819da10dR121

40k is actually the max precision the cardinality aggregation supports

Yep, that's why I wanted some indication in case the reader of the code is unaware; this way they'll understand the choice and not think it's some arbitrary magic number. 👍

justinkambic · 2020-01-07T19:18:04Z

x-pack/legacy/plugins/uptime/server/lib/adapters/monitor_states/search/query_context.ts

+    }
+
+    // @ts-ignore
+    const tsStart = DateMath.parse(this.dateRangeEnd).subtract(10, 'seconds');


I can see us potentially wanting to make the window size an API parameter in the future.

justinkambic · 2020-01-07T19:19:36Z

x-pack/legacy/plugins/uptime/server/lib/adapters/monitor_states/search/query_context.ts

+                },
+                {
+                  bool: {
+                    must_not: { exists: { field: 'monitor.timespan' } },


TIL must_not ignores scoring.

justinkambic · 2020-01-07T19:27:58Z

x-pack/test/api_integration/apis/uptime/rest/snapshot.ts

-      );
-      expectFixtureEql(apiResponse.body, 'snapshot');
-    });
+    const dateRangeStart = new Date().toISOString();


~~Having looked at the internals and now seeing these declarations, I think we can eventually specify just one date (end time) and a number of seconds to subtract from it for our API calls.~~

I'm realizing this is false - we still need the full window size for things like date histograms.

x-pack/test/api_integration/apis/uptime/rest/snapshot.ts

justinkambic

LGTM WFG!

kibanamachine · 2020-01-09T14:25:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 34bafdc

History

💔 Build #18890 failed 6ab02cb
💔 Build #18877 failed a117d95
💔 Build #18847 failed 40b0429
💔 Build #18817 failed b394a19e8535ad24e18d3b84a442ad280974f148
💔 Build #18753 failed 6b8b258

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…#52433) This PR optimizes both the snapshot component and the monitor list on the overview page by using the new monitor.timespan field from elastic/beats#14778. Note that the functionality here will work with heartbeats lacking that patch, but the performance improvements will be absent. This PR adapts the snapshot tests to use synthetically generated data which should be easier to maintain. As a result some of that code is refactored as well. See elastic#52433 parent issue as well.

…#54352) This PR optimizes both the snapshot component and the monitor list on the overview page by using the new monitor.timespan field from elastic/beats#14778. Note that the functionality here will work with heartbeats lacking that patch, but the performance improvements will be absent. This PR adapts the snapshot tests to use synthetically generated data which should be easier to maintain. As a result some of that code is refactored as well. See #52433 parent issue as well.

andrewvc added 3 commits December 6, 2019 13:26

Progress

af39194

Merge remote-tracking branch 'origin/master' into timespan-optimization

daf0488

Checkpoint

526cc47

andrewvc added enhancement New value added to drive a business result Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Dec 6, 2019

andrewvc self-assigned this Dec 6, 2019

andrewvc added 3 commits December 8, 2019 10:54

Checkpoint

2725ba5

Checkpoint

45276a2

Faster counts of snapshots

3e818b4

andrewvc added the WIP Work in progress label Dec 10, 2019

andrewvc requested a review from justinkambic December 10, 2019 02:33

andrewvc added 2 commits December 9, 2019 20:34

Reformat

b039c9c

Merge remote-tracking branch 'origin/master' into timespan-optimization

39bcc8d

andrewvc added release_note:enhancement v7.6.0 labels Dec 10, 2019

andrewvc added 6 commits December 10, 2019 11:58

Remove all references to mixed state

5acd892

Use generated data for snapshot tests

b428ae3

Timespan tests work

ff0a77c

Fix TS

8c645ff

Checkpoint

eff678e

Fixes

6766a20

andrewvc changed the title ~~Timespan optimization~~ [Uptime] Improve query performance with Heartbeat 7.6+ data. Dec 11, 2019

andrewvc added review and removed WIP Work in progress labels Dec 11, 2019

This was referenced Dec 11, 2019

[META] Improve performance of overview queries via range data elastic/uptime#123

Closed

[Heartbeat] Add monitor.timespan field elastic/beats#14778

Merged

Tidy up

1e9413b

shahzad31 reviewed Dec 12, 2019

View reviewed changes

x-pack/legacy/plugins/uptime/server/lib/helper/make_date_rate_filter.ts Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into timespan-optimization

b17c223

andrewvc added 4 commits January 6, 2020 13:55

Fix rest API tests

61c61d9

Merge remote-tracking branch 'origin/master' into timespan-optimization

df70eb7

Merge remote-tracking branch 'origin/master' into timespan-optimization

9bbf210

Update snapshots

1eb4a1a

justinkambic reviewed Jan 7, 2020

View reviewed changes

andrewvc added 7 commits January 7, 2020 16:11

Pass dataTestSubj as prop

5d789d0

Fix tests

d5911ba

Merge remote-tracking branch 'origin/master' into timespan-optimization

5321a3d

Fix ts ignore

6b8b258

Merge remote-tracking branch 'origin/master' into timespan-optimization

5dd83c5

Fix typing issues

fe1e5c7

Merge remote-tracking branch 'origin/master' into HEAD

4eb1ab2

andrewvc force-pushed the timespan-optimization branch from b394a19 to 4eb1ab2 Compare January 8, 2020 20:55

andrewvc added 4 commits January 8, 2020 15:06

Fix i18n

40b0429

Fix style

5dcef2a

Fix namespacing

a117d95

Fix data-test-subj

6ab02cb

justinkambic approved these changes Jan 9, 2020

View reviewed changes

Fix snapshots

34bafdc

andrewvc merged commit 2d15b8c into elastic:master Jan 9, 2020

andrewvc deleted the timespan-optimization branch January 9, 2020 14:36

andrewvc mentioned this pull request Jan 9, 2020

[7.x] [Uptime] Improve query performance with Heartbeat 7.6+ data. (#52433) #54352

Merged

brianseeders mentioned this pull request Jan 9, 2020

Revert "[Uptime] Monitor details page left side title (#53529)" #54394

Closed

justinkambic mentioned this pull request Jan 9, 2020

[Uptime] Fix broken functional tests in master #54395

Merged

andrewvc removed their assignment Jan 16, 2020

andrewvc added the test-plan label Jan 16, 2020

andrewvc mentioned this pull request Jan 31, 2020

[Uptime] Add 'Stale' monitor state #56465

Closed

[Uptime] Improve query performance with Heartbeat 7.6+ data. #52433

[Uptime] Improve query performance with Heartbeat 7.6+ data. #52433

Uh oh!

Conversation

andrewvc commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Dec 6, 2019

Uh oh!

Uh oh!

justinkambic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Chart Legend Row

Chart Legend

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinkambic Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinkambic left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 9, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andrewvc commented Dec 6, 2019 •

edited

Loading

justinkambic Jan 7, 2020 •

edited

Loading