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] Moving file data vizualizer to its own plugin #96408

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Apr 7, 2021

Moves ML's file data visualizer component into it's own plugin fileDataVisualizer and then imports it back into ML for use in the Data Visualizer section of the app.

Note, very little new code has been added, all UI code in this PR has just been moved or copied from the ML plugin.

The file data visualizer component relies on a decent amount of code shared with the index data visualizer which has had to be duplicated into the new plugin, along with some common ML utility functions and types.

The feature should function as before with some exceptions.

  • Boolean field types are missing their preview charts as it would require copying a lot of code, this will be fixed in a follow up PR.
  • Links to ML pages have been remove after a file has been successfully uploaded. If we still want this these links, they will have to be added in a follow up PR. Plugins which use this component will have to supply the results links to be embedded.

Also adds index_exists and time_field_range endpoints to the fileUpload plugin to assist with importing data, moves the analyze_file endpoint from fileUpload to fileDataVisualizer

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

retest

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

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

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

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

@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

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

@qn895
Copy link
Member

qn895 commented Apr 18, 2021

Code LGTM 🎉

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM.

Would be good to get the boolean type preview chart showing up in the collapsed row, but happy for that to be done in a separate PR.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

There are a lot of duplicated components between file data visualizer and index pattern visualizer. Should fileDataVisualizer expose these components so they can then be used in ML for the index pattern visualizer? Or would it make sense to move index pattern visualizer into this plugin as well?

@@ -10,7 +10,7 @@ import React, { FC } from 'react';

import { EuiTitle, EuiSpacer } from '@elastic/eui';

import { MLJobEditor, ML_EDITOR_MODE } from '../../../../jobs/jobs_list/components/ml_job_editor';
import { MLJobEditor, ML_EDITOR_MODE } from '../ml_job_editor';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these loose the ML prefix?


import { i18n } from '@kbn/i18n';

import { getMLJobTypeAriaLabel } from '../../util/field_types_utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should getMLJobTypeAriaLabel lose the ML prefix?

this.maxFileUploadBytes = getFileUpload().getMaxBytes();

this.savedObjectsClient = props.savedObjectsClient;
this.maxFileUploadBytes = this.props.fileUpload.getMaxBytes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: use 'props' instead of 'this.props' to be consistent with line above

@@ -0,0 +1,8 @@
.mlEmbeddedMapContent {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should ml prefix be removed?

} from '../../../../../../../src/plugins/embeddable/public';
import { useFileDataVisualizerKibana } from '../../kibana_context';

export function MlEmbeddedMapComponent({
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be called EmbeddedMapComponent?

statusCode: number;
error: string;
message: string;
attributes?: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about replacing any with a type definition?

): Promise<boolean> {
const body = JSON.stringify({ index });
const fileUploadModules = await lazyLoadModules();
return await fileUploadModules.getHttp().fetch<any>({
Copy link
Contributor

Choose a reason for hiding this comment

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

How about replacing any with a type definition for check upload resposne?

});
}

export async function getTimeFieldRange(index: string, query: any, timeFieldName?: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can query be unknown instead of any?

client: IScopedClusterClient,
index: string[] | string,
timeFieldName: string,
query: any
Copy link
Contributor

Choose a reason for hiding this comment

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

Can query be unknown instead of any?

* 2.0.
*/
import { IScopedClusterClient } from 'kibana/server';
export async function getTimeFieldRange(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is getTimeFieldRange endpoint even needed? Why not just use already exposed ES search endpoint and have a client function that contains this logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes i agree, the original ML version of this function takes more arguments and so I think justifies having it's own endpoint. i've stripped most of these out for the file_upload version as they're not needed.
I'll move this search to the client side in a follow up PR.

@jgowdyelastic
Copy link
Member Author

@nreese

Or would it make sense to move index pattern visualizer into this plugin as well?

Seeing as this new plugin is decoupled from the file upload process, it might make sense to move the index data visualizer in here too.
There are plans on ML's road map to move it out so it can be embedded into discover.
I think for this first version it would be ok to have these components duplicated, If we decide to move the index data visualizer into this plugin (and rename plugin) we can revert some of the changes i've made to separate them out.
If we don't decide to move it in here, I agree that making the components shared between plugins would be good.

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@nreese nreese 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 moving file data visualizer into its own plugin. This is a great first step to integrating in other Kibana plugins.

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fileDataVisualizer - 178 +178
ml 1803 1732 -71
total +107

Async chunks

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

id before after diff
fileDataVisualizer - 1.0MB ⚠️ +1.0MB
fileUpload 789.1KB 789.1KB +5.0B
ml 6.0MB 5.9MB -155.6KB
total +881.5KB

Page load bundle

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

id before after diff
fileDataVisualizer - 11.8KB +11.8KB
fileUpload 14.0KB 15.0KB +1014.0B
ml 68.9KB 68.5KB -409.0B
total +12.4KB
Unknown metric groups

API count

id before after diff
fileDataVisualizer - 19 +19
fileUpload 180 115 -65
total -46

API count missing comments

id before after diff
fileDataVisualizer - 19 +19
fileUpload 180 115 -65
total -46

API count with any type

id before after diff
fileUpload 6 4 -2

async chunk count

id before after diff
fileDataVisualizer - 1 +1

History

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

cc @jgowdyelastic

@jgowdyelastic jgowdyelastic added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 20, 2021
@jgowdyelastic jgowdyelastic changed the title [ML] Moving file data vizualizer to file upload plugin [ML] Moving file data vizualizer to its own plugin Apr 20, 2021
@jgowdyelastic jgowdyelastic merged commit 088a618 into elastic:master Apr 20, 2021
@jgowdyelastic jgowdyelastic deleted the moving-file-data-visualizer-UI-to-file-upload-plugin branch April 20, 2021 21:17
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Apr 20, 2021
* [ML] Moving file data vizualizer to file upload plugin

* removing maps plug dependency

* fixing imports

* small refactor

* adding missing endpoints

* fixing translations

* fxing table controls

* fixing types and disabling geo point test

* actually disabling geo point test

* making endpoints internal

* moving UI code to separate plugin

* enabling maps integration

* cleaning up dependencies

* fixing translation ids

* moving analyze file endpoint out of file upload plugin

* fixing transtations issues

* refactor for lazy loading of component

* updating limits

* updating plugin asciidoc

* code clean up

* further clean up

* adding comment

* fixing really obvious CI error

* removing commented out include

* reenabling geo point test

* fixing incorrectly changed import

* removing ml from labels and identifiers

* renaming function

* moving analyse file endpoint to file upload plugin

* reverting import path changes

* adding esUiShared back in

* fixing navigation tabs alignment in basic license

* adding key to tab wrapper

* reverting test label

* further removal of ml references

* removing ml label from more identifiers

* fixing tests

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 20, 2021
* [ML] Moving file data vizualizer to file upload plugin

* removing maps plug dependency

* fixing imports

* small refactor

* adding missing endpoints

* fixing translations

* fxing table controls

* fixing types and disabling geo point test

* actually disabling geo point test

* making endpoints internal

* moving UI code to separate plugin

* enabling maps integration

* cleaning up dependencies

* fixing translation ids

* moving analyze file endpoint out of file upload plugin

* fixing transtations issues

* refactor for lazy loading of component

* updating limits

* updating plugin asciidoc

* code clean up

* further clean up

* adding comment

* fixing really obvious CI error

* removing commented out include

* reenabling geo point test

* fixing incorrectly changed import

* removing ml from labels and identifiers

* renaming function

* moving analyse file endpoint to file upload plugin

* reverting import path changes

* adding esUiShared back in

* fixing navigation tabs alignment in basic license

* adding key to tab wrapper

* reverting test label

* further removal of ml references

* removing ml label from more identifiers

* fixing tests

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: James Gowdy <[email protected]>
jgowdyelastic added a commit that referenced this pull request May 24, 2023
Adds versioning to all of the file upload APIs.
Versions are added to the server side routes and to the client side
functions which call the routes.
Updates API tests to add the API version to the request headers.

All of the APIs are internal and have been given the version '1'.

Also renames `/internal/file_data_visualizer/analyze_file` to
`/internal/file_upload/analyze_file`
It appears this was a mistake from when the route was moved from the
data visualiser plugin midway through development on [this
PR](#96408).

**Internal APIs**

`/internal/file_upload/analyze_file`
`/internal/file_upload/has_import_permission`
`/internal/file_upload/index_exists`
`/internal/file_upload/time_field_range`

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:File and Index Data Viz ML file and index data visualizer :ml non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants