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

[ML] Transforms: Migrate IndexPattern service usage to DataView service #128247

Merged

Conversation

peteharverson
Copy link
Contributor

Summary

The DataViews service has been broken out of the data plugin into the data_views plugin. This PR replaces all usage of the now deprecated indexPatterns API in the transform plugin with the DataView API.

As well as updating imports, most of the uses of indexPattern type parameter and function names have been replaced to dataView.

Note that related changes to functional test function names have been kept to a minimum to reduce the code churn in this PR.

Part of #124063 and #124092

Checklist

@peteharverson peteharverson added review :ml technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes Feature:Transforms ML transforms backport:skip This commit does not require backporting v8.2.0 labels Mar 22, 2022
@peteharverson peteharverson self-assigned this Mar 22, 2022
@peteharverson peteharverson requested a review from a team as a code owner March 22, 2022 11:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@qn895
Copy link
Member

qn895 commented Mar 22, 2022

Tested and LGTM 🎉

@@ -5,18 +5,18 @@
* 2.0.
*/

import type { IndexPattern } from '../../../../../src/plugins/data/common';
import { DataView } from '../../../../../src/plugins/data_views/common';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep the type part here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b846d83

@@ -8,7 +8,7 @@
import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey';

import { HttpFetchError } from '../../../../../../src/core/public';
import type { IndexPattern } from '../../../../../../src/plugins/data/public';
import { DataView } from '../../../../../../src/plugins/data_views/public';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep the type part here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b846d83

const isMissingFields = resp.hits.hits.every((d) => typeof d.fields === 'undefined');

const docs = resp.hits.hits.map((d) => getProcessedFields(d.fields ?? {}));

// Get all field names for each returned doc and flatten it
// to a list of unique field names used across all docs.
const allKibanaIndexPatternFields = getFieldsFromKibanaIndexPattern(indexPattern);
const allKibanaDataViewFields = getFieldsFromKibanaIndexPattern(dataView);
Copy link
Contributor

Choose a reason for hiding this comment

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

KibanaIndexPattern was used to distinguish between raw string based index patterns and a Kibana maintained index pattern object. So suggest to now name this just allDataViewFields since DataView is a Kibana-only thing anyway. I see getFieldsFromKibanaIndexPattern is brought over from the ml plugin so this will be done in another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to allDataViewFields in b846d83. Yes I have left the remaining IndexPattern to DataView function / variable renaming in the ml plugin for a follow-up :)

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Code LGTM, just added small nit suggestions.

@kibana-ci
Copy link
Collaborator

💚 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
transform 375.0KB 374.7KB -356.0B
Unknown metric groups

References to deprecated APIs

id before after diff
transform 140 1 -139

History

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

cc @peteharverson

@peteharverson peteharverson merged commit 337fadf into elastic:main Mar 24, 2022
@peteharverson peteharverson deleted the transforms-data-views-service branch March 24, 2022 12:28
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:Transforms ML transforms :ml release_note:skip Skip the PR/issue when compiling release notes review technical debt Improvement of the software architecture and operational architecture v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants