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] migrate file_data_visualizer/import route to file_upload plugin #89640

Merged
merged 26 commits into from Feb 2, 2021

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jan 28, 2021

This PR starts the process of extracting ml file_data_visualizer into a reusable file_upload plugin. This PR starts small and just extracts import route. PR also extracts all typings, schemas, utility functions, and and telemetry.

@nreese nreese requested a review from a team as a code owner January 28, 2021 21:22
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@nreese
Copy link
Contributor Author

nreese commented Jan 28, 2021

@elasticmachine merge upstream

@nreese nreese requested a review from a team as a code owner January 28, 2021 23:56
@@ -74,8 +74,8 @@ export async function updateTelemetry(internalRepo?: ISavedObjectsRepository) {

function incrementCounts(telemetry: Telemetry) {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not using SavedObjectsRepository.incrementCounter?

Recently we've added support to increment multiple fields with custom increments:

incrementCounter<T = unknown>(type: string, id: string, counterFields: Array<string | SavedObjectsIncrementCounterField>, options?: SavedObjectsIncrementCounterOptions): Promise<SavedObject<T>>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Bamieh This PR is migrating code from ML to a separate plugin, trying to change as little as possible. I can create an issue to track using SavedObjectsRepository.incrementCounter but would like to leave any refactoring out of this initial file_upload creation pull request.

@@ -1771,10 +1771,14 @@
}
}
},
"fileUploadTelemetry": {
"fileUpload": {
Copy link
Member

Choose a reason for hiding this comment

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

Can we merge both telemetry types? fileUpload and fileUploadTelemetry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The next step of the process of migrating ML file_upload plugin will be to remove the maps_file_upload plugin. That will remove will fileUploadTelemetry type. For the new file_upload plugin, I wanted to start with a clean namespace to avoid any problems processing telemetry for the old plugin.

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

Core changes LGTM (including telemetry). I added a couple of questions before approving

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.

LGTM. Tested latest edits, and confirmed the issue around telemetry has been resolved, so I was able to successfully import files from the ML file data visualizer page.

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@nreese
Copy link
Contributor Author

nreese commented Feb 2, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1734 1736 +2

Async chunks

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

id before after diff
mapsFileUpload 825.7KB 825.8KB +5.0B
ml 6.6MB 6.6MB +2.1KB
total +2.1KB

Page load bundle

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

id before after diff
ml 57.2KB 57.1KB -45.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
file-upload-usage-collection-telemetry - 3 +3
ml-telemetry 3 - -3
total -0

History

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

@nreese nreese merged commit d601090 into elastic:master Feb 2, 2021
nreese added a commit to nreese/kibana that referenced this pull request Feb 2, 2021
…lastic#89640)

* migrate file_upload plugin to maps_file_upload

* update plugins list

* migrate ml import endpoint

* migrate ml telemetry to file_upload plugin

* add fileUpload plugin to ml

* add TS project

* update ML to use file_upload endpoint

* move types to file_upload plugin

* ignore error

* clean up

* i18n clean-up

* remove schemas from ml

* remove usageCollection from ml

* node scripts/build_plugin_list_docs

* update telemety collector

* revert changes to ingestPipeline schema

* change name of TELEMETRY_DOC_ID to unique value

* remove ImportFile from ml/server/routes/apidoc.json

* fix typo in x=pack/tsconfig.json

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/telemetry/schema/oss_plugins.json
#	x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json
nreese added a commit to nreese/kibana that referenced this pull request Feb 2, 2021
…lastic#89640)

* migrate file_upload plugin to maps_file_upload

* update plugins list

* migrate ml import endpoint

* migrate ml telemetry to file_upload plugin

* add fileUpload plugin to ml

* add TS project

* update ML to use file_upload endpoint

* move types to file_upload plugin

* ignore error

* clean up

* i18n clean-up

* remove schemas from ml

* remove usageCollection from ml

* node scripts/build_plugin_list_docs

* update telemety collector

* revert changes to ingestPipeline schema

* change name of TELEMETRY_DOC_ID to unique value

* remove ImportFile from ml/server/routes/apidoc.json

* fix typo in x=pack/tsconfig.json

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/telemetry/schema/oss_plugins.json
#	x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json
phillipb added a commit to phillipb/kibana that referenced this pull request Feb 2, 2021
…-ml-jobs

* 'master' of github.com:elastic/kibana: (254 commits)
  [Security Solution] [Detections] Remove allow_no_indices to prevent error being thrown in response of field capabilities (elastic#89927)
  Skip test for cloud (elastic#89450)
  [Fleet] Fix duplicate data streams being shown in UI (elastic#89812)
  Bump package dependencies (elastic#90034)
  [App Search] DRY helper for encoding/decoding routes that can have special characters in params (elastic#89811)
  TypeScript project references for Observability plugin (elastic#89320)
  [SearchSource] Combine sort and parent fields when serializing (elastic#89808)
  Made imports static (elastic#89935)
  [ml] migrate file_data_visualizer/import route to file_upload plugin (elastic#89640)
  [Discover] Adapt default column behavior (elastic#89826)
  Round start and end values (elastic#89030)
  Rename getProxyAgents to getCustomAgents (elastic#89813)
  [Form lib] UseField `onError` listener (elastic#89895)
  [APM] use latency sum instead of avg for impact (elastic#89990)
  migrate more core-owned plugins to tsproject ref (elastic#89975)
  [Logs UI] Load <LogStream> entries via async searches (elastic#86899)
  [APM] Abort browser requests when appropriate (elastic#89557)
  [Alerting] Allow user to select existing connector of same type when fixing broken connector (elastic#89062)
  [Data Table] Use shared CSV export mechanism (elastic#89702)
  chore(NA): improve logic check when installing Bazel tools (elastic#89634)
  ...
nreese added a commit that referenced this pull request Feb 2, 2021
…89640) (#90075)

* migrate file_upload plugin to maps_file_upload

* update plugins list

* migrate ml import endpoint

* migrate ml telemetry to file_upload plugin

* add fileUpload plugin to ml

* add TS project

* update ML to use file_upload endpoint

* move types to file_upload plugin

* ignore error

* clean up

* i18n clean-up

* remove schemas from ml

* remove usageCollection from ml

* node scripts/build_plugin_list_docs

* update telemety collector

* revert changes to ingestPipeline schema

* change name of TELEMETRY_DOC_ID to unique value

* remove ImportFile from ml/server/routes/apidoc.json

* fix typo in x=pack/tsconfig.json

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/telemetry/schema/oss_plugins.json
#	x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants