Move Rollup out of legacy #62891

cjcenizal · 2020-04-08T00:12:45Z

This PR complements #21117, in which the client was moved out of legacy. This PR completes the migration by moving the server code out of legacy, too.

Other changes

Per discussion with @lukasolson and @flash1293, I attempted to remove the rollup search strategy code (649b172). This code was deprecated in favor of functionality provided by the data_enhanced plugin. However, this ended up breaking some TSVB functional tests so I reverted this change.

Reviewing

Reviewing each commit individually might be easier than reviewing the end result. I tried to make each one as atomic as possible. Note that the changes to the rollup search strategy in the first commit can be ignored because this code was removed in a subsequent commit.

Testing

Setup

The pattern for creating a rollup job and rollup index pattern is:

Install sample data (web logs is a good one).
Create a rollup job with an index pattern that captures this index (e.g. k*).
Set frequency to "minute". Clear the latency buffer field.
Select the time field which is the same time field selected in the installed index pattern (timestamp without an @ in the case of web logs).
Select a few terms, histogram, and metrics fields.
Create and start the rollup job. Wait a minute for the job to run. You should see the numbers for documents and pages processed change in the detail panel.
Create a rollup index pattern in the Index Patterns app.
Now you can create visualizations using this index pattern.

Settings

Set xpack.rollup.enabled: false and confirm the app isn't visible in the UI.

Create job

Validation

Verify that a validation error tells you when the index pattern doesn't match any indices.

Verify that a validation error tells you when there's a collision between the index pattern and rollup index name.

Verify that a validation error tells you when the index pattern matches rollup indices.
Verify that a success message tells you when the index pattern matches indices.

Verify that a validation error tells you that you need to input a time bucket size.

Review

Verify that the "Request" tab shows you the request you can make to create the rollup job you've configured.

Verify that checking the "Start now" checkbox creates the rollup job in a "Started" state.

Actions

Stop a job and confirm its "Delete" action isn't available.
Start a job and confirm its "Delete" action is available.
Delete a job.
Select a started job and a stopped job. Confirm that performing the "Start" option on them starts the stopped one and there are no errors.
Select a started job and a stopped job. Confirm that performing the "Stop" option on them starts the stopped one and there are no errors.
Select two started jobs and perform the "Stop" action on them.
Select two stopped jobs and perform the "Start" action on them.
Select two started jobs and confirm you can't "Delete" them. Stop them and then delete them.

Job configuration

Create a rollup job without terms, histogram, or metrics fields and confirm these tabs aren't visible in the job's detail panel after it's been created.

Missing entity

Open a detail flyout of a rollup job and then edit the URL so that it loads one that doesn't exist. Verify it shows a "not found" message.

Errors

Some of these errors are reproducible through the UI (e.g. creating jobs with name collisions), but some require editing the API code to send incorrect payloads to ES. I don't think reviewers need to test them -- I'm just recording this information to show that I tested these cases and verified that changes to the way the API returns errors haven't broken the way the UI handles them.

elasticmachine · 2020-04-08T00:13:06Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

elasticmachine · 2020-04-23T16:03:02Z

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

- Note incorrect metric type for tracking requests.

…hanced plugin now provides this functionality.

cjcenizal · 2020-04-23T16:45:36Z

@lukasolson @flash1293 Could you please confirm that the rollup search strategy code I removed in db4c9797f2a0beb4201f9ad732ca55105636e61d hasn't broken anything on the Kibana App side?

… endpoint.

cjcenizal · 2020-04-24T00:13:18Z

@flash1293 Can you take a look at my changes to the TSVB rollup tests in 5f9937d? These were originally failing because the metric text was 0, instead 3 as expected. What's odd is that as I watched the test run locally I noticed the time field was never being selected -- so I guess the question I have isn't as much what broke, but how were these originally passing? I'll take a look on master to compare behavior.

Edit: I checked out master and, yup, something in this PR has changed the way TSVB behaves when consuming rollup data. I'm not familiar enough with TSVB to lend any insight here.

… data_enhanced plugin now provides this functionality." This reverts commit 649b172.

This reverts commit 5f9937d.

cjcenizal · 2020-04-24T00:32:53Z

@lukasolson @flash1293 Sorry for the noise. 😅 I tried reverting my commit that removes the rollup search strategy and that got the tests passing again. So now the only things I need your eyes on are the changes to the rollup search strategy code in the original NP migration commit, 9434083.

…p search endpoint." This reverts commit 9e190e6.

flash1293

Tested visualize and TSVB and both still work fine, LGTM. No code review. @lukasolson probably knows better how the search endpoints line up behind the scenes.

alisonelizabeth

Nice work @cjcenizal! Thanks for the detailed PR description - very helpful. I went through all the test cases and everything worked as expected. I left two non-blocker comments. Great to see another plugin 100% migrated 🎉

alisonelizabeth · 2020-04-24T17:24:52Z

x-pack/plugins/rollup/server/plugin.ts

+        request: KibanaRequest
+      ): APICaller => {
+        return rollupEsClient.asScoped(request).callAsCurrentUser;
+        // return (...args: any[]): APICaller => {


can this be deleted?

Yes, good spot!

alisonelizabeth · 2020-04-24T17:31:18Z

x-pack/plugins/rollup/tsconfig.json

@@ -0,0 +1,3 @@
+{


is this file necessary?

kibanamachine · 2020-04-24T20:28:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 187e5ac

History

💚 Build #42958 succeeded 4b3295c
💚 Build #42948 succeeded 93d013f
💔 Build #42937 failed 9e190e6
💔 Build #42811 failed bcb526f
💔 Build #40082 failed 29e23ad0c060010e27c34ce6a7081ec4bac3c15d

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

rylnd · 2020-04-27T23:34:41Z

@cjcenizal does this still need to be backported to 7.x? I wasn't able to find a backport PR and my own NP backport conflicts with this, so I'm guessing so.

I created #64603 which is likely going to cause a conflict with your eventual 7.x backport (sorry!), but it should just be two lines in x-pack/index.js.

cjcenizal · 2020-04-28T00:00:06Z

Thanks for the nudge @rylnd! Looks like I did miss the backport. 🤕

* Move Rollup out of legacy (#62891) * Put back 7.x-specific logic.

cjcenizal added chore Feature:New Platform Feature:Rollups release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.8.0 v8.0.0 labels Apr 8, 2020

elastic deleted a comment from kibanamachine Apr 10, 2020

cjcenizal force-pushed the np/rollup-out-of-legacy branch from 2d780ee to 29e23ad Compare April 10, 2020 02:15

cjcenizal changed the title ~~Move Rollup out of legacy~~ [skip-ci] Move Rollup out of legacy Apr 10, 2020

cjcenizal added Feature:NP Migration and removed Feature:New Platform labels Apr 16, 2020

cjcenizal force-pushed the np/rollup-out-of-legacy branch 7 times, most recently from 8bee653 to b861ca8 Compare April 23, 2020 16:01

cjcenizal changed the title ~~[skip-ci] Move Rollup out of legacy~~ Move Rollup out of legacy Apr 23, 2020

cjcenizal added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Apr 23, 2020

cjcenizal marked this pull request as ready for review April 23, 2020 16:03

cjcenizal requested a review from a team as a code owner April 23, 2020 16:03

cjcenizal added 4 commits April 23, 2020 09:25

Convert server to NP.

9434083

Update paths in README and code comments.

b41de04

- Note incorrect metric type for tracking requests.

Remove rollup search strategy and rollup search endpoint. The data_en…

649b172

…hanced plugin now provides this functionality.

Internationalize rollup prompt callout.

bcb526f

cjcenizal force-pushed the np/rollup-out-of-legacy branch from b861ca8 to bcb526f Compare April 23, 2020 16:26

cjcenizal requested review from flash1293, lukasolson and alisonelizabeth April 23, 2020 16:44

cjcenizal added 2 commits April 23, 2020 16:36

Fix API integration tests by removing tests for deleted rollup search…

9e190e6

… endpoint.

Fix TSVB rollup functional tests.

5f9937d

cjcenizal added 2 commits April 23, 2020 17:25

Revert "Remove rollup search strategy and rollup search endpoint. The…

226a04a

… data_enhanced plugin now provides this functionality." This reverts commit 649b172.

Revert "Fix TSVB rollup functional tests."

93d013f

This reverts commit 5f9937d.

Revert "Fix API integration tests by removing tests for deleted rollu…

4b3295c

…p search endpoint." This reverts commit 9e190e6.

flash1293 approved these changes Apr 24, 2020

View reviewed changes

alisonelizabeth approved these changes Apr 24, 2020

View reviewed changes

Remove commented-out code and unnecessary tsconfig file.

187e5ac

cjcenizal merged commit 18d1af2 into elastic:master Apr 24, 2020

cjcenizal deleted the np/rollup-out-of-legacy branch April 24, 2020 21:27

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Apr 28, 2020

Move Rollup out of legacy (elastic#62891)

db2acd4

cjcenizal mentioned this pull request Apr 28, 2020

[7.x] Move Rollup out of legacy (#62891) #64608

Merged

cjcenizal added a commit that referenced this pull request Apr 28, 2020

[7.x] Move Rollup out of legacy (#62891) (#64608)

ecdbece

* Move Rollup out of legacy (#62891) * Put back 7.x-specific logic.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Rollup out of legacy #62891

Move Rollup out of legacy #62891

cjcenizal commented Apr 8, 2020 •

edited

Loading

elasticmachine commented Apr 8, 2020

elasticmachine commented Apr 23, 2020

cjcenizal commented Apr 23, 2020

cjcenizal commented Apr 24, 2020 •

edited

Loading

cjcenizal commented Apr 24, 2020

flash1293 left a comment •

edited

Loading

alisonelizabeth left a comment

alisonelizabeth Apr 24, 2020

cjcenizal Apr 24, 2020

alisonelizabeth Apr 24, 2020

kibanamachine commented Apr 24, 2020

rylnd commented Apr 27, 2020

cjcenizal commented Apr 28, 2020

Move Rollup out of legacy #62891

Move Rollup out of legacy #62891

Conversation

cjcenizal commented Apr 8, 2020 • edited Loading

Other changes

Reviewing

Testing

Setup

Settings

Create job

Validation

Review

Actions

Job configuration

Missing entity

Errors

elasticmachine commented Apr 8, 2020

elasticmachine commented Apr 23, 2020

cjcenizal commented Apr 23, 2020

cjcenizal commented Apr 24, 2020 • edited Loading

cjcenizal commented Apr 24, 2020

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

alisonelizabeth left a comment

Choose a reason for hiding this comment

alisonelizabeth Apr 24, 2020

Choose a reason for hiding this comment

cjcenizal Apr 24, 2020

Choose a reason for hiding this comment

alisonelizabeth Apr 24, 2020

Choose a reason for hiding this comment

kibanamachine commented Apr 24, 2020

💚 Build Succeeded

History

rylnd commented Apr 27, 2020

cjcenizal commented Apr 28, 2020

cjcenizal commented Apr 8, 2020 •

edited

Loading

cjcenizal commented Apr 24, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading