Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[data / data views] Disable rollup functionality when rollup plugin is disabled #162674

Merged
merged 35 commits into from
Aug 8, 2023

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Jul 28, 2023

Summary

Rollup functionality will be disabled on serverless instances. The rollup plugin will be disabled. While it won't be possible to create rollups on serverless, it will be possible to import data view saved objects which may be configured for use with rollups. We will make a 'best effort' to improve functionality as to help users transition away from rollups.

  • In classic environments, the rollup plugin will enable dataViews and data plugin rollup functionality.
  • The data plugin will run an async search instead of a rollup search.
  • The data views plugin will fetch normal fields, omitting a query for rollup fields - which would fail anyway.

Closes #152708

@mattkime mattkime changed the title partial progress [data / data views] Disable rollup functionality when rollup plugin is disabled Aug 3, 2023
@mattkime mattkime self-assigned this Aug 3, 2023
@mattkime mattkime marked this pull request as ready for review August 3, 2023 19:35
@mattkime mattkime requested a review from a team as a code owner August 3, 2023 19:35
Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this issue, @mattkime!
I have created a rollup index and a data view for it with a stateful Kibana, exported the saved object for the data view and imported it into a serverless Kibana. Are those steps correct to reproduce the situation? Or should I also import the "rollup" index into serverless? Because otherwise the index pattern doesn't match anything.
Also, I'd prefer to keep #152708 open. With this PR merged, we still have risks associated with the usage of rollups api by the data view plugin.
Do you know if there are any api integration tests for data view plugin that checks the rollups api?

@mattkime
Copy link
Contributor Author

mattkime commented Aug 4, 2023

@yuliacech

Or should I also import the "rollup" index into serverless?

As best I know thats not possible.

Because otherwise the index pattern doesn't match anything.

I was under the impression that the common use case for rollup data is to view it along side non rollup data. Even if thats not true, I think there's value in being able to import the assets so that they can be modified or replaced.

With this PR merged, we still have risks associated with the usage of rollups api by the data view plugin.

Can you be more specific? I'm not aware of any such risks.

Do you know if there are any api integration tests for data view plugin that checks the rollups api?

No, I should do that. I guess the best route would be to see if a rollup data view could load its fields. It should error if it tries to hit a rollup endpoint.

@yuliacech
Copy link
Contributor

@mattkime I think when an api endpoint is used by a different plugin, especially if our team doesn't own this other plugin, we might accidentally cause a regression for this other plugin. The dependency between plugins is an implicit one, our team didn't know about the data view plugin until recently. There are no TS types to rely on either.
When our team needs to change the endpoint or remove it completely to accommodate changes to the rollup plugin's client side, we might not realize the dependency since it's not documented anywhere. If there are no tests on the data view side that checks this endpoint, the changes to the endpoint and any potential regressions will not be known to the other team either.

@mattkime
Copy link
Contributor Author

mattkime commented Aug 4, 2023

@yuliacech

If there are no tests on the data view side that checks this endpoint, the changes to the endpoint and any potential regressions will not be known to the other team either.

There are functional tests for rollup data view functionality - https://github.com/elastic/kibana/blob/main/x-pack/test/api_integration/apis/management/rollup/index_patterns_extensions.js and https://github.com/elastic/kibana/blob/main/x-pack/test/functional/apps/rollup_job/hybrid_index_pattern.js

Since rollups are a deprecated feature, I'd prefer to do as little work on them as possible.

@yuliacech
Copy link
Contributor

I agree about rollups being deprecated and low priority at the moment. Just to be on the safe side, I'll add a comment to the rollup endpoint linking to #152708

mockApiCaller.mockResolvedValueOnce(mockRollupResponse);
mockSubmitCaller.mockResolvedValueOnce(mockAsyncResponse);

const params = { index: 'foo-程', body: { query: {} } };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, 程 means journey? right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to, it was duplicated from the previous test.

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thx for taking care of this and handling rollups gracefully. Tested in server & serverless and worked as expeced. Also thx for adding serverless tests. Can be merged in once they're green

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2573 2575 +2
dataViews 243 246 +3
total +5
Unknown metric groups

API count

id before after diff
data 3295 3297 +2
dataViews 1012 1016 +4
total +6

ESLint disabled line counts

id before after diff
dataViews 12 11 -1

Total ESLint disabled count

id before after diff
dataViews 14 13 -1

History

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

cc @mattkime

@mattkime mattkime merged commit a69870b into elastic:main Aug 8, 2023
@kibanamachine kibanamachine added v8.10.0 backport:skip This commit does not require backporting labels Aug 8, 2023
yuliacech added a commit that referenced this pull request Aug 8, 2023
## Summary
Adds a comment that the API endpoint /rollup/indices is used by the data
view plugin.

With #162674 merged, the issue
#152708 has been closed. The
Data Discovery team is not planning any further work around rollup api
use due to the feature being deprecated. I think adding a comment to the
API code directly will help us remember this dependency for when we are
going to edit or remove the endpoint.
@davismcphee
Copy link
Contributor

@mattkime should we assign x-pack/test_serverless/api_integration/test_suites/common/rollups.ts to us in CODEOWNERS?

bryce-b pushed a commit to bryce-b/kibana that referenced this pull request Aug 9, 2023
…s disabled (elastic#162674)

## Summary

Rollup functionality will be disabled on serverless instances. The
`rollup` plugin will be disabled. While it won't be possible to create
rollups on serverless, it will be possible to import data view saved
objects which may be configured for use with rollups. We will make a
'best effort' to improve functionality as to help users transition away
from rollups.

- In classic environments, the `rollup` plugin will enable `dataViews`
and `data` plugin rollup functionality.
- The data plugin will run an async search instead of a rollup search.
- The data views plugin will fetch normal fields, omitting a query for
rollup fields - which would fail anyway.

Closes elastic#152708

---------

Co-authored-by: kibanamachine <[email protected]>
bryce-b pushed a commit to bryce-b/kibana that referenced this pull request Aug 9, 2023
…#163432)

## Summary
Adds a comment that the API endpoint /rollup/indices is used by the data
view plugin.

With elastic#162674 merged, the issue
elastic#152708 has been closed. The
Data Discovery team is not planning any further work around rollup api
use due to the feature being deprecated. I think adding a comment to the
API code directly will help us remember this dependency for when we are
going to edit or remove the endpoint.
mattkime added a commit that referenced this pull request Jun 5, 2024
…#184765)

## Summary

Previously we were checking the `/api/rollup/indices` endpoint when the
data view editor was loaded. If rollups were disabled, it would
harmlessly handle a 404 error. Still, people justifiably wonder whether
something is wrong.

This PR skips checking the endpoint as one would expect. A unit test has
been added. I also manually verified the behavior is as expected.

Follow up to #162674

---------

Co-authored-by: Lukas Olson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Rollups release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data view editor plugin uses Rollup Jobs internal API endpoint
9 participants