[TSVB] use new Search API for rollup search by alexwizp · Pull Request #83275 · elastic/kibana

alexwizp · 2020-11-12T13:03:20Z

Summary

Historically, the code related to the rollup search in TSVB was added to the x-pack/rollup plugin. And now the rollup plugin has a folder specific only to TSVB (see x-pack/plugins/rollup/server/lib/search_strategies)
IMO, this is a design issue because usually a TSVB plugin should depend on a Rollup plugin, but not vice versa.

On the other hand, in #76274, we started using a new Search API that natively supports search by rollup data.

I think we should:

check which logic we still need from x-pack/plugins/rollup/server/lib/search_strategies and which can be reused from the data plugin
create a new vis_type_timeseries_enhanced plugin and move into it related code
use new Search API for searching by Rollup data
related code was migrated js -> ts

During testing the following issues was found on master branch: #83414. It's not related to that changes

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)
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

alexwizp · 2020-11-12T14:08:59Z

@elasticmachine merge upstream

Closes: elastic#82710

…st.ts

alexwizp · 2020-11-14T11:05:33Z

@elasticmachine merge upstream

alexwizp · 2020-11-16T09:09:43Z

@elasticmachine merge upstream

elasticmachine · 2020-11-16T12:14:31Z

Pinging @elastic/kibana-app (Team:KibanaApp)

flash1293 · 2020-11-16T14:06:36Z

Didn't dig into it, but it seems like something doesn't work right in the table vis:

alexwizp · 2020-11-16T14:36:44Z

@flash1293 no sure that this issues somehow related to my changes. I see that for some cases ES return object instead of numeric values and I'm 99.9% sure that on master branch we have the same behavior.
The main difference with master I removed rest_total_hits_as_int param. I think it should be added in data plugin. But this flag is not related to that problem. I've checked that.

some proofs:

flash1293 · 2020-11-16T14:39:27Z

Tested and it works on master:

If this is caused by something within the data plugin, let's open a separate issue and fix it first there, then come back to this change.

alexwizp · 2020-11-16T15:40:49Z

Previous issue was discussed offline with @flash1293. In master we have the same issue which is not related to that changes.
New issues was created to address that problem: #83436

cjcenizal

👏 👏 So happy to see this tech debt being addressed! I remember being very confused earlier this year about where the rollup search strategy was being consumed. The indirection made it a challenge to track down. This makes things much clearer.

I only reviewed the removed code in rollup, which all LGTM. Didn't test locally.

stratoula · 2020-11-17T07:22:12Z

@alexwizp can you also add the kibana-app team as codeowners of the new plugin?

alexwizp · 2020-11-18T07:55:18Z

@elasticmachine merge upstream

src/plugins/vis_type_timeseries/server/lib/vis_data/helpers/unit_to_seconds.ts

x-pack/plugins/vis_type_timeseries_enhanced/README.md

stratoula

LGTM, thank you @alexwizp I really like this PR 😄

kibanamachine · 2020-11-18T13:56:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 52f43f4

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	42849	42850	+1

History

💚 Build #88497 succeeded 88320ba
💚 Build #88233 succeeded 850da04
💚 Build #87957 succeeded d4e943b
💔 Build #87946 failed 9da44655c692391d6b7c6096c2f53e63ea1ffa9d
💚 Build #87918 succeeded 55bdbbd

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

* [TSVB] use new Search API for rollup search Closes: elastic#82710 * remove unused code * rollup_search_strategy.test.js -> rollup_search_strategy.test.ts * default_search_capabilities.test.js -> default_search_capabilities.test.ts * remove getRollupService * fix CI * fix some types * update types * update codeowners * fix PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # .github/CODEOWNERS # x-pack/plugins/vis_type_timeseries_enhanced/server/search_strategies/rollup_search_capabilities.test.ts

…o-node-details * 'master' of github.com:elastic/kibana: (65 commits) update chromedriver dependency to 87 (elastic#83624) [TSVB] use new Search API for rollup search (elastic#83275) [TSVB] Y-axis has number formatting not considering all series formatters in the group (elastic#83438) [Logs UI] Update <LogStream /> internal state when its props change (elastic#83302) Add tag bulk action context menu (elastic#82816) [code coverage] adding plugin to flush coverage data (elastic#83447) [UsageCollection] Expose `KibanaRequest` to explicitly opted-in collectors (elastic#83413) Added eventBus to trigger and listen plotHandler event (elastic#83435) [Runtime fields] Editor phase 1 (elastic#81472) [Maps] Fix threshold alert issue resolving nested fields (elastic#83577) chore(NA): remove usage of unverified es snapshots (elastic#83589) [DOCS] Adds Elastic Contributor Program link (elastic#83561) Upgrade EUI to v30.2.0 (elastic#82730) Don't show loading screen during auto-reload (elastic#83376) Functional tests - fix esArchive mappings with runtime fields (elastic#83530) [deb/rpm] Create keystore after installation (elastic#76465) [rpm] Create default environment file at "/etc/sysconfig/kibana" (elastic#82144) [docker] removes workaround for missing crypto-policies-scripts subpackage (elastic#83455) [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439) adds xpack.security.authc.selector.enabled setting (elastic#83551) ...

* [TSVB] use new Search API for rollup search Closes: #82710 * remove unused code * rollup_search_strategy.test.js -> rollup_search_strategy.test.ts * default_search_capabilities.test.js -> default_search_capabilities.test.ts * remove getRollupService * fix CI * fix some types * update types * update codeowners * fix PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # .github/CODEOWNERS # x-pack/plugins/vis_type_timeseries_enhanced/server/search_strategies/rollup_search_capabilities.test.ts

alexwizp force-pushed the 82710 branch from 748c4f9 to 5ff3add Compare November 12, 2020 13:10

alexwizp changed the title ~~[TSVB] use new Search API for rollup search~~ [WIP][TSVB] use new Search API for rollup search Nov 12, 2020

alexwizp force-pushed the 82710 branch from 5ff3add to bdc4ae2 Compare November 12, 2020 13:44

alexwizp force-pushed the 82710 branch 3 times, most recently from 41ea328 to 1eff1c8 Compare November 12, 2020 16:46

elastic deleted a comment from kibanamachine Nov 12, 2020

alexwizp added 4 commits November 13, 2020 14:28

[TSVB] use new Search API for rollup search

0be5797

Closes: elastic#82710

remove unused code

4dcf3a4

rollup_search_strategy.test.js -> rollup_search_strategy.test.ts

5cd4024

default_search_capabilities.test.js -> default_search_capabilities.te…

dbe9609

…st.ts

alexwizp force-pushed the 82710 branch from 2a3f731 to dbe9609 Compare November 13, 2020 12:13

remove getRollupService

488fe5b

kibanamachine and others added 3 commits November 14, 2020 06:05

Merge branch 'master' into 82710

6f0cc30

fix CI

20d51cb

fix some types

af81f5c

Merge branch 'master' into 82710

55bdbbd

alexwizp force-pushed the 82710 branch from bbf8d10 to 9da4465 Compare November 16, 2020 11:13

update types

d4e943b

alexwizp force-pushed the 82710 branch from 9da4465 to d4e943b Compare November 16, 2020 12:03

alexwizp changed the title ~~[WIP][TSVB] use new Search API for rollup search~~ [TSVB] use new Search API for rollup search Nov 16, 2020

alexwizp requested a review from stratoula November 16, 2020 12:11

alexwizp self-assigned this Nov 16, 2020

alexwizp added v7.11.0 v8.0.0 Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Feature:TSVB TSVB (Time Series Visual Builder) labels Nov 16, 2020

alexwizp added the Feature:Rollups label Nov 16, 2020

alexwizp requested a review from cjcenizal November 16, 2020 12:14

alexwizp marked this pull request as ready for review November 16, 2020 12:14

alexwizp requested a review from a team as a code owner November 16, 2020 12:14

alexwizp requested a review from a team November 16, 2020 12:14

alexwizp added release_note:enhancement technical debt Improvement of the software architecture and operational architecture labels Nov 16, 2020

cjcenizal approved these changes Nov 16, 2020

View reviewed changes

alexwizp added 2 commits November 17, 2020 11:39

update codeowners

67988ab

Merge remote-tracking branch 'upstream/master' into 82710

850da04

Merge branch 'master' into 82710

88320ba

stratoula reviewed Nov 18, 2020

View reviewed changes

src/plugins/vis_type_timeseries/server/lib/vis_data/helpers/unit_to_seconds.ts Outdated Show resolved Hide resolved

stratoula reviewed Nov 18, 2020

View reviewed changes

x-pack/plugins/vis_type_timeseries_enhanced/README.md Outdated Show resolved Hide resolved

fix PR comments

52f43f4

stratoula approved these changes Nov 18, 2020

View reviewed changes

alexwizp merged commit 7114db3 into elastic:master Nov 18, 2020

alexwizp mentioned this pull request Nov 18, 2020

[7.x] [TSVB] use new Search API for rollup search (#83275) #83639

Merged

alexwizp deleted the 82710 branch January 16, 2021 15:27

Conversation

alexwizp commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

alexwizp commented Nov 12, 2020

Uh oh!

alexwizp commented Nov 14, 2020

Uh oh!

alexwizp commented Nov 16, 2020

Uh oh!

elasticmachine commented Nov 16, 2020

Uh oh!

flash1293 commented Nov 16, 2020

Uh oh!

alexwizp commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Nov 16, 2020

Uh oh!

alexwizp commented Nov 16, 2020

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

stratoula commented Nov 17, 2020

Uh oh!

alexwizp commented Nov 18, 2020

Uh oh!

Uh oh!

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Nov 18, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alexwizp commented Nov 12, 2020 •

edited

Loading

alexwizp commented Nov 16, 2020 •

edited

Loading