Move to modified version of original status count for speed #41210

andrewvc · 2019-07-16T01:24:56Z

Summary

This attempts to speed up the now slow snapshot count @justinkambic noticed. It uses a different approach form #41203, it's actually a modified version of the old formula. It should be faster. It achieves this executing a query that's very friendly to composite aggs, even if it returns more data than strictly necessary.

This also fixes a bug in the original implementation, where the total would be equivalent to the filtered value, rather than the pre-filtered value. This made little sense since you'd just see the total twice.

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-07-16T02:22:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-16T04:09:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-16T17:17:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-16T17:35:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-16T18:28:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-16T19:07:28Z

Pinging @elastic/uptime

justinkambic

This functionality seems to work for me, and I like the improvement to the way we express total.

I'm looking for a little discussion on some of my comments but aside from that this looks great.

Thank you for tackling this important task.

justinkambic · 2019-07-16T13:32:00Z

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

    let searchAfter: any = null;

+    const summaryByIdLocation: {


We can probably extract a type for this and put it in the adjacent adapter_types file. A separate type for the location value would probably be cleaner too.

justinkambic · 2019-07-16T13:54:36Z

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

-            down++;
-          }
+        const source: any = get(bucket, 'latest.hits.hits[0]._source');
+        const {


Are these keys guaranteed to be defined?

Yes, aggregations always return a value.

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

justinkambic · 2019-07-16T20:01:17Z

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

+      }
+    }
+
+    return result;


Do we have a unit test for this function? It may be good to create a mock JSON result (from real-world query results) and a subsequent test snapshot to help ensure that future revisions don't break this function, since we're planning to make it GA and we've developed it so late.

If you agree then I'm happy to add this, or do it in a follow-up PR. WDYT?

For this specific case I don't see a ton of value in the unit test. Our functional tests check this against real query results from an ES cluster. That said it'd be nice to have the unit test to make future development against this code easier, since running functional tests is a pain.

I might make more of a meta-style issue to try to address this. Once we've stabilized these backend functions it'll be important to ensure that future changes don't erode the functionality.

++ to have a style discussion elsewhere. When/where to unit test is a complex question with no one single answer, and no shortage of different philosophies.

justinkambic · 2019-07-16T20:02:13Z

x-pack/test/api_integration/apis/uptime/graphql/fixtures/snapshot_filtered_by_down.json

@@ -1,5 +1,5 @@
 {
  "snapshot": {
-    "counts": { "down": 2, "mixed": 0, "up": 0, "total": 2 }
+    "counts": { "down": 2, "mixed": 0, "up": 0, "total": 10 }


I like that we're doing this now. I think. 😺

I would categorize the previous behavior as a bug IMHO.

…ticsearch_monitors_adapter.ts Co-Authored-By: Justin Kambic <justin.kambic@elastic.co>

elasticmachine · 2019-07-16T22:42:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

justinkambic

LGTM WFG

elasticmachine · 2019-07-16T23:55:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

@justinkambic

…41210) This attempts to speed up the now slow snapshot count @justinkambic noticed. It uses a different approach form elastic#41203, it's actually a modified version of the old formula. It should be faster. It achieves this executing a query that's very friendly to composite aggs, even if it returns more data than strictly necessary. This also fixes a bug in the original implementation, where the total would be equivalent to the filtered value, rather than the pre-filtered value. This made little sense since you'd just see the total twice.

@justinkambic

…41210) This attempts to speed up the now slow snapshot count @justinkambic noticed. It uses a different approach form elastic#41203, it's actually a modified version of the old formula. It should be faster. It achieves this executing a query that's very friendly to composite aggs, even if it returns more data than strictly necessary. This also fixes a bug in the original implementation, where the total would be equivalent to the filtered value, rather than the pre-filtered value. This made little sense since you'd just see the total twice.

@justinkambic

…41323) This attempts to speed up the now slow snapshot count @justinkambic noticed. It uses a different approach form #41203, it's actually a modified version of the old formula. It should be faster. It achieves this executing a query that's very friendly to composite aggs, even if it returns more data than strictly necessary. This also fixes a bug in the original implementation, where the total would be equivalent to the filtered value, rather than the pre-filtered value. This made little sense since you'd just see the total twice.

@justinkambic

…41325) This attempts to speed up the now slow snapshot count @justinkambic noticed. It uses a different approach form #41203, it's actually a modified version of the old formula. It should be faster. It achieves this executing a query that's very friendly to composite aggs, even if it returns more data than strictly necessary. This also fixes a bug in the original implementation, where the total would be equivalent to the filtered value, rather than the pre-filtered value. This made little sense since you'd just see the total twice.

Patching this function in elastic#41210 fixed performance but was missing this filter clause. We depend on only processing summary info. This fixes errors around missing destructured data where we expect summary fields to be present.

Patching this function in #41210 fixed performance but was missing this filter clause. We depend on only processing summary info. This fixes errors around missing destructured data where we expect summary fields to be present.

…1335) Patching this function in elastic#41210 fixed performance but was missing this filter clause. We depend on only processing summary info. This fixes errors around missing destructured data where we expect summary fields to be present.

) (#41424) * [Uptime] Handle `mode: all` in uptime snapshot calculation (#41335) Patching this function in #41210 fixed performance but was missing this filter clause. We depend on only processing summary info. This fixes errors around missing destructured data where we expect summary fields to be present. * Update snapshot to use heartbeat7.0.0 index for backport

) (#41423) * [Uptime] Handle `mode: all` in uptime snapshot calculation (#41335) Patching this function in #41210 fixed performance but was missing this filter clause. We depend on only processing summary info. This fixes errors around missing destructured data where we expect summary fields to be present. * Update snapshot to use heartbeat7.0.0 index for backport

Move to modified version of original status count for speed

36cda82

andrewvc added bug Fixes for quality problems that affect the customer experience review [zube]: In Review labels Jul 16, 2019

andrewvc requested a review from justinkambic July 16, 2019 01:24

andrewvc self-assigned this Jul 16, 2019

Fix types

48d106b

andrewvc added 2 commits July 16, 2019 10:03

Use _source to return minimal fields

76e4ac2

Merge remote-tracking branch 'origin/master' into fast-status-count

56e97fb

andrewvc added the release_note:skip Skip the PR/issue when compiling release notes label Jul 16, 2019

Fix test fixtures

bff50a3

andrewvc added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Jul 16, 2019

justinkambic reviewed Jul 16, 2019

View reviewed changes

Update x-pack/legacy/plugins/uptime/server/lib/adapters/monitors/elas…

ec236ce

…ticsearch_monitors_adapter.ts Co-Authored-By: Justin Kambic <justin.kambic@elastic.co>

andrewvc added 2 commits July 16, 2019 17:49

Use map reduce

a9ac880

fix types

a83a19d

justinkambic approved these changes Jul 16, 2019

View reviewed changes

justinkambic mentioned this pull request Jul 16, 2019

[Uptime] Update monitor states queries to enhance performance #41203

Closed

7 tasks

andrewvc merged commit 9ecadb2 into elastic:master Jul 17, 2019

andrewvc deleted the fast-status-count branch July 17, 2019 00:09

zube bot removed the [zube]: In Review label Jul 17, 2019

zube bot added the [zube]: Done label Jul 17, 2019

andrewvc mentioned this pull request Jul 17, 2019

[7.x] Move to modified version of original status count for speed (#41210) #41323

Merged

andrewvc mentioned this pull request Jul 17, 2019

[7.3] Move to modified version of original status count for speed (#41210) #41325

Merged

andrewvc mentioned this pull request Jul 17, 2019

[Uptime] Handle mode: all in uptime snapshot calculation #41335

Merged

7 tasks

andrewvc removed the [zube]: Done label Jul 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to modified version of original status count for speed #41210

Move to modified version of original status count for speed #41210

andrewvc commented Jul 16, 2019 •

edited

Loading

elasticmachine commented Jul 16, 2019

elasticmachine commented Jul 16, 2019

elasticmachine commented Jul 16, 2019

elasticmachine commented Jul 16, 2019

elasticmachine commented Jul 16, 2019

elasticmachine commented Jul 16, 2019

justinkambic left a comment

justinkambic Jul 16, 2019

justinkambic Jul 16, 2019

andrewvc Jul 16, 2019

justinkambic Jul 16, 2019

andrewvc Jul 16, 2019

justinkambic Jul 16, 2019

andrewvc Jul 16, 2019

justinkambic Jul 16, 2019

andrewvc Jul 16, 2019

elasticmachine commented Jul 16, 2019

justinkambic left a comment

elasticmachine commented Jul 16, 2019

Move to modified version of original status count for speed #41210

Move to modified version of original status count for speed #41210

Conversation

andrewvc commented Jul 16, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jul 16, 2019

💔 Build Failed

elasticmachine commented Jul 16, 2019

💔 Build Failed

elasticmachine commented Jul 16, 2019

💔 Build Failed

elasticmachine commented Jul 16, 2019

💔 Build Failed

elasticmachine commented Jul 16, 2019

💚 Build Succeeded

elasticmachine commented Jul 16, 2019

justinkambic 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 Jul 16, 2019

💔 Build Failed

justinkambic left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 16, 2019

💚 Build Succeeded

andrewvc commented Jul 16, 2019 •

edited

Loading