[Partial Results] Move other bucket into Search Source by lizozom · Pull Request #96384 · elastic/kibana

lizozom · 2021-04-07T10:11:16Z

Summary

Depends on #96241

When an aggregation has an other bucket, one or more post flight requests are needed to construct it.
Previously, that logic was part of the request handler code, making it available to expressions, but not to direct consumers of SearchSource.

This PR:

Allows passing an AggConfigs object into searchSource.setField('aggs', ...).
Removes the boolean parameter passed to aggConfigs.toDsl
Adds a hierarchical option to AggConfigs, replacing the previous parameter
Moves the handling of postFlightRequests into search source.

Response

The search observable returned from searchSource.fetch$() generally emits one or more responses (all but the last one being partial results). If the AggConfigs passed to the searchSource generate postFlightRequests, the searchSource will emit an additional single response (even if it required multiple calls), with the other bucket fully populated.

Release Note

Adds Other Bucket support to SearchSource, by passing in an AggConfigs object directly to searchSource.setField('aggs', aggs).

The search observable returned from searchSource.fetch$() will emit one or more responses (all but the last one being partial results). If the AggConfigs passed to the searchSource generate postFlightRequests, the searchSource will emit an additional single response (even if it required multiple calls), with the other bucket fully populated.

Tests

These changes effect any consumers of SearchSource, with or without the Other bucket, i.e. apps like Dashboard, Visualizations, Lens and Maps. Consumers of the lower level data.search API should not be effected by this PR.

Special attention should be given to testing the Other bucket, hierarchical parameter (formerly known as metricsAtAllLevels) and the behavior of the inspector in the above scenarios.

Unit tests

_terms_other_bucket_helper.test.ts
- Added types
request_handler.test.ts
- 'setField(aggs)' - adjust aggs type
- 'calls searchSource.fetch' - adjust inspector format
- ~~'calls agg.postFlightRequest if it exiests and agg is enabled'~~
- ~~'should skip agg.postFlightRequest call if the agg is disabled'~~
- tabifies response data - removed metricsAtAllLevels
search_source.test.ts
- Fixed wrong nesting of isPartial and isRunning in response mock
- Typing of aggs required adjustments
- Use fetch$ instead of deprecated fetch
- sets the value for the property with AggConfigs
- fetch$
  - Removed duplicate legacy msearch tests and grouped others
  - shareReplays result
  - should emit error on empty response
  - inspector
    - calls inspector if provided
    - calls inspector only once, with multiple subs (shareReplay)
    - calls error on inspector
  - postFlightRequest
    - doesnt call any post flight requests if disabled
    - doesnt call any post flight if searchsource has error
    - calls post flight requests, fires 1 extra response, returns last response
    - calls post flight requests, handles error
esaggs.test.ts
- request format change (client and server)
agg_configs.test.ts
- Use hierarchical: true

Functional tests

Added a functional test that uses search source to send a bucket aggregation and validates the presence of the other bucket in the final response.

Checklist

Delete any items that are not applicable to this PR.

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

…tor-integration-in-search-source

…ther-bucket

lizozom · 2021-04-09T15:31:39Z

@elasticmachine merge upstream

kibanamachine · 2021-04-09T15:31:41Z

merge conflict between base and head

…ther-bucket

lizozom · 2021-04-11T09:41:00Z

src/plugins/data/common/search/aggs/buckets/_terms_other_bucket_helper.ts

-  if (responseAgg[aggWithOtherBucket.id]) {
-    return responseAgg[aggWithOtherBucket.id].buckets;
+  if (responseAgg?.[aggWithOtherBucket.id]) {
+    return (responseAgg[aggWithOtherBucket.id] as any).buckets;


Introduced response typing, but I don't want to broaden the refactoring of this code

ts improvements more inspector tests

more tests

…ther-bucket

lizozom · 2021-04-14T12:01:44Z

@elasticmachine merge upstream

ryankeairns · 2021-04-14T13:51:27Z

@lizozom can you add a screenshot of the resulting UI? It will help with the quick design review, thanks.

lizozom · 2021-04-14T21:02:36Z

@ryankeairns I don't think this deserves a review honestly.
It's just a demo plugin we use for automated testing, but I was bothered by the text padding 😅

…ther-bucket

…into search/move-other-bucket

…ther-bucket

lizozom · 2021-04-16T15:07:18Z

@elasticmachine merge upstream

…ther-bucket

…into search/move-other-bucket

* Move inspector adapter integration into search source * docs and ts * Move other bucket to search source * test ts + delete unused tabilfy function * hierarchical param in aggconfig. ts improvements more inspector tests * fix jest * separate inspect more tests * jest * inspector * Error handling and more tests * put the fun in functional tests * code review * Add functional test for other bucket in search example app * test * test * ts * test * test * ts Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-04-18T09:52:53Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* Move inspector adapter integration into search source * docs and ts * Move other bucket to search source * test ts + delete unused tabilfy function * hierarchical param in aggconfig. ts improvements more inspector tests * fix jest * separate inspect more tests * jest * inspector * Error handling and more tests * put the fun in functional tests * code review * Add functional test for other bucket in search example app * test * test * ts * test * test * ts Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Liza Katz <lizka.k@gmail.com>

kibanamachine · 2021-04-18T12:16:13Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	407.5KB	407.2KB	-336.0B
`maps`	2.6MB	2.6MB	-184.0B
total			-520.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	788.9KB	789.8KB	+880.0B

Unknown metric groups

API count

id	before	after	diff
`data`	3438	3440	+2

API count missing comments

id	before	after	diff
`data`	2946	2945	-1

API count with any type

id	before	after	diff
`data`	81	79	-2

History

💚 Build #120369 succeeded d098ce6
💔 Build #120315 failed c3f0872
💔 Build #120308 failed d3f2836
💔 Build #120299 failed 56a2d82
💔 Build #120284 failed 49e72a0

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

cc @lizozom

…te-legacy-es-client * 'master' of github.com:elastic/kibana: (102 commits) [Exploratory view] integerate page views to exploratory view (elastic#97258) Fix typo in license_api_guard README name and import http server mocks from public interface (elastic#97334) Avoid mutating KQL query when validating it (elastic#97081) Add description as title on tag badge (elastic#97109) Remove legacy ES client usages in `home` and `xpack_legacy` (elastic#97359) [Fleet] Finer-grained error information from install/upgrade API (elastic#95649) Rule registry bundle size (elastic#97251) [Partial Results] Move other bucket into Search Source (elastic#96384) [Dashboard] Makes lens default editor for creating new panels (elastic#96181) skip flaky suite (elastic#97387) [Asset Management] Agent picker follow up (elastic#97357) skip flaky suite (elastic#97382) [Security Solutions] Fixes flake with cypress tests (elastic#97329) skip flaky suite (elastic#97355) Skip test to try and stabilize master minimize number of so fild asserted in tests. it creates flakines when implementation details change (elastic#97374) [Search Sessions] Client side search cache (elastic#92439) [SavedObjects] Add aggregations support (elastic#96292) [Reporting] Remove legacy elasticsearch client usage from the reporting plugin (elastic#97184) [kbnClient] fix basePath handling and export reponse type (elastic#97277) ... # Conflicts: # x-pack/plugins/watcher/server/lib/license_pre_routing_factory/license_pre_routing_factory.ts # x-pack/plugins/watcher/server/plugin.ts # x-pack/plugins/watcher/server/routes/api/indices/register_get_route.ts # x-pack/plugins/watcher/server/routes/api/license/register_refresh_route.ts # x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts # x-pack/plugins/watcher/server/routes/api/register_load_history_route.ts # x-pack/plugins/watcher/server/routes/api/settings/register_load_route.ts # x-pack/plugins/watcher/server/routes/api/watch/action/register_acknowledge_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_activate_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_deactivate_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_delete_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_execute_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_history_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_load_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_save_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_visualize_route.ts # x-pack/plugins/watcher/server/routes/api/watches/register_delete_route.ts # x-pack/plugins/watcher/server/routes/api/watches/register_list_route.ts # x-pack/plugins/watcher/server/shared_imports.ts # x-pack/plugins/watcher/server/types.ts

Liza K added 5 commits April 5, 2021 21:50

Move inspector adapter integration into search source

173b1b9

docs and ts

d3c9dbb

Merge branch 'master' of github.com:elastic/kibana into search/inspec…

f927075

…tor-integration-in-search-source

Merge branch 'master' of github.com:elastic/kibana into search/inspec…

f44c320

…tor-integration-in-search-source

Move other bucket to search source

413b462

lizozom requested review from lukeelmers and ppisljar April 7, 2021 10:11

lizozom added Team:AppServices v7.13.0 v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes labels Apr 7, 2021

Liza K added 2 commits April 7, 2021 16:30

test ts + delete unused tabilfy function

7862457

Merge branch 'master' of github.com:elastic/kibana into search/move-o…

1bd2887

…ther-bucket

Merge branch 'master' of github.com:elastic/kibana into search/move-o…

f22bab9

…ther-bucket

lizozom commented Apr 11, 2021

View reviewed changes

Liza K added 9 commits April 11, 2021 12:47

hierarchical param in aggconfig.

b9301dd

ts improvements more inspector tests

fix jest

e88df9d

separate inspect

47b9944

more tests

jest

b9ce734

inspector

2976277

Error handling and more tests

8d4aafe

Merge branch 'master' of github.com:elastic/kibana into search/move-o…

f2d202e

…ther-bucket

Merge branch 'master' of github.com:elastic/kibana into search/move-o…

f5e9a1b

…ther-bucket

put the fun in functional tests

c7514ee

lizozom marked this pull request as ready for review April 13, 2021 08:10

lizozom requested a review from a team April 13, 2021 08:10

lizozom requested a review from a team as a code owner April 13, 2021 08:10

Add functional test for other bucket in search example app

164319b

lizozom requested a review from a team as a code owner April 14, 2021 10:33

Merge branch 'master' into search/move-other-bucket

1c60934

Merge branch 'master' of github.com:elastic/kibana into search/move-o…

cde09cf

…ther-bucket

ryankeairns approved these changes Apr 14, 2021

View reviewed changes

Liza K added 8 commits April 15, 2021 18:40

test

01b0f26

test

5ba1d9e

Merge branch 'search/move-other-bucket' of github.com:lizozom/kibana …

0b89ce3

…into search/move-other-bucket

ts

c39309d

test

e6f9042

Merge branch 'master' of github.com:elastic/kibana into search/move-o…

4cab9fb

…ther-bucket

test

49e72a0

ts

56a2d82

kibanamachine and others added 4 commits April 16, 2021 11:07

Merge branch 'master' into search/move-other-bucket

d3f2836

Merge branch 'master' into search/move-other-bucket

c3f0872

Merge branch 'master' of github.com:elastic/kibana into search/move-o…

95e6acf

…ther-bucket

Merge branch 'search/move-other-bucket' of github.com:lizozom/kibana …

d098ce6

…into search/move-other-bucket

lizozom merged commit 1a3e033 into elastic:master Apr 18, 2021

kibanamachine mentioned this pull request Apr 18, 2021

[7.x] [Partial Results] Move other bucket into Search Source (#96384) #97397

Merged

timroes added the Feature:Search Querying infrastructure in Kibana label Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Partial Results] Move other bucket into Search Source#96384

[Partial Results] Move other bucket into Search Source#96384
lizozom merged 34 commits intoelastic:masterfrom
lizozom:search/move-other-bucket

lizozom commented Apr 7, 2021 •

edited

Loading

Uh oh!

lizozom commented Apr 9, 2021

Uh oh!

kibanamachine commented Apr 9, 2021

Uh oh!

lizozom Apr 11, 2021

Uh oh!

lizozom commented Apr 14, 2021

Uh oh!

ryankeairns commented Apr 14, 2021

Uh oh!

lizozom commented Apr 14, 2021

Uh oh!

lizozom commented Apr 16, 2021

Uh oh!

kibanamachine commented Apr 18, 2021

Uh oh!

kibanamachine commented Apr 18, 2021

API count

API count missing comments

API count with any type

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

lizozom commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Response

Release Note

Tests

Unit tests

Functional tests

Checklist

For maintainers

Uh oh!

lizozom commented Apr 9, 2021

Uh oh!

kibanamachine commented Apr 9, 2021

Uh oh!

lizozom Apr 11, 2021

Choose a reason for hiding this comment

Uh oh!

lizozom commented Apr 14, 2021

Uh oh!

ryankeairns commented Apr 14, 2021

Uh oh!

lizozom commented Apr 14, 2021

Uh oh!

lizozom commented Apr 16, 2021

Uh oh!

kibanamachine commented Apr 18, 2021

💚 Backport successful

Uh oh!

kibanamachine commented Apr 18, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

API count

API count missing comments

API count with any type

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lizozom commented Apr 7, 2021 •

edited

Loading