[File upload] Add file upload x-pack plugin #32945

kindsun · 2019-03-11T22:47:05Z

Adds file upload as a plugin, original functionality pulled from ML file upload. Accomplishes the following:

Create x-pack plugin for file upload
Create endpoint for file upload
Update telemetry to track apps submitting files and types of files uploaded
Add new telemetry tests
Bubble up ES warnings to users for poorly formatted data submissions

elasticmachine · 2019-03-11T23:34:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-12T15:03:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-12T23:39:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kobelb · 2019-03-13T14:20:59Z

x-pack/plugins/file_upload/common/constants/file_import.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export const MAX_BYTES = 104857600;


100mB is a rather large default, we read the entire request into memory and then duplicate it's contents to another variable before doing the bulk insert in Elasticsearch, this will require 200mB additional heap. We'll likely want to make this smaller by default and configurable, or split this up into multiple bulk insert operations (which would alleviate these constraints).

Maybe reduce it to 10 MB initially and adjust as needed?

That seems reasonable to me.

kobelb · 2019-03-13T14:28:07Z

x-pack/plugins/file_upload/server/client/call_with_request_factory.js

+import { once } from 'lodash';
+
+const callWithRequest = once((server) => {
+  const cluster = server.plugins.elasticsearch.createCluster('fileupload');


Why are we using a dedicated cluster here as opposed to the 'data' cluster?

Looks like a historical decision that I copied over. I'll switch it over to the data cluster

kobelb · 2019-03-13T16:12:29Z

x-pack/plugins/file_upload/server/models/file_data_visualizer/import_data.js

+
+
+function generateId() {
+  return Math.random().toString(36).substr(2, 9);


I'd recommend using a uuid here instead of our own id generation.

kobelb · 2019-03-13T16:23:00Z

x-pack/plugins/file_upload/server/telemetry/telemetry.ts

+  return new SavedObjectsClient(internalRepository);
+}
+
+export async function incrementFileDataVisualizerIndexCreationCount(server: Server): Promise<void> {


I'd recommend emulating what KQL's telemetry endpoint is doing here: https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/server/routes/api/kql_telemetry/index.js#L35

Ok, planning to do another pass on telemetry. I'll give this a look!

Sorry for the belated follow up! I diverted for some time to the related #34748 which focuses on the front-end portion. I needed this context to determine how well the plugin would work in practice for full round-trips.

I did end up revising the telemetry to be more similar to what kql was doing but I'm certain it could use another pass. I think (hopefully) we're in a reasonable spot now but it's worth nothing that we'll end up iterating on telemetry at least one more round (and maybe more) for #35809.

elasticmachine · 2019-03-14T00:55:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

… and file types

elasticmachine · 2019-03-26T01:20:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-26T14:00:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kindsun · 2019-05-01T21:53:04Z

@peteharverson Thanks for correcting me and getting @jgowdyelastic and @walterra on board, glad to have you both reviewing as well!

The MAX_BYTES limit here is applied per upload chunk, therefore it would be possible to upload 100MB in 10 chunks.

@jgowdyelastic Ah right. Thanks for correcting me there

Bumping the limit to 25MB might be safer.

@jgowdyelastic Done. I actually bumped it to 30 MB. This will likely change in the future but hopefully that number will work well for now. Also @peteharverson, I realized it might not be as configurable on the fly as I'd imagined (very likely by design) as that value is assigned to the config portion of the route at route initialization.

@walterra Would love to get your feedback on the telemetry. As I mentioned to Pete above, it will likely change and get more detail on future iterations, but hopefully we're close to being in a good spot for now.

General question. Would it be worth creating an issue to move ML file upload over to using this plugin following its merge with master? We could then delete the redundant code from ML. I only ask because this question comes up frequently on the GIS team. We just don't want to introduce redundant code in the form of a plugin that only we use. I'd be happy to create the issue and potentially the PR as well when the time comes.

elasticmachine · 2019-05-01T21:53:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-01T23:03:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-02T00:42:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-02T01:49:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

peteharverson · 2019-05-02T11:09:56Z

@aaronjcaldwell I just created a new issue #35947 to switch the ML file data visualizer over to using the new file upload plugin and remove the redundant code from ML. Once your PR is merged, we will schedule the work in - we are happy to do the changes ourselves.

walterra

Code in general LGTM, just added a comment about the telemetry data structure.

walterra · 2019-05-02T12:11:43Z

x-pack/plugins/file_upload/server/telemetry/telemetry.ts

+    },
+    filesUploadedByApp: {
+      ...filesUploadedByApp,
+      [app]: {


As far as I understand how the server collecting the data works, this data structure using app as part of the field names will require to update the mapping and possibly reindex data every time a new type of app gets added here. Same for fileType above. So it should be fine to use this structure, it's just something to be aware of.

To make this more obvious for future developers coming across this it might be worth adding a comment and typing app and fileType more strictly for example like app: 'maps' | 'ml'.

I'd suggest verifying this with the telemetry team because I'm not 100% sure I remember all of this correctly.

Makes sense to me! I've added some comments above the dynamic fields. Resolution of the docs issue shortly after this should help as well. I've also created an issue for the telemetry team's review.

elasticmachine · 2019-05-02T15:10:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kindsun · 2019-05-02T16:28:49Z

retest

elasticmachine · 2019-05-02T17:22:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-03T20:41:01Z

Pinging @elastic/kibana-gis

# Conflicts: # x-pack/index.js

elasticmachine · 2019-05-07T15:30:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kindsun · 2019-05-07T17:19:35Z

retest

elasticmachine · 2019-05-07T18:31:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kindsun · 2019-05-07T19:58:39Z

@kobelb @jgowdyelastic @peteharverson I think we should be in reasonable shape for approval on this PR but please let me know if there's anything else you'd like to see updated or changed!

@walterra Still waiting to hear back on telemetry structure but I don't think that should necessarily hold us up in terms of merging the overall plugin. We'll be iterating on telemetry regardless. Let me know what you think!

Just a general FYI, I'm not planning to merge this until #34748 is ready to merge as well. It's not so much a blocker as a "context". We absolutely wanted ML's expertise for review on this portion of the plugin, but we don't want to merge the plugin until the Maps UI portion is also approved, at that point we'll merge them both back-to-back. Hopefully that makes sense but happy to discuss if needed!

jgowdyelastic

LGTM

walterra

Sounds good, LGTM 🙌

kindsun · 2019-05-10T14:21:43Z

After some internal discussion, we've elected to move this plugin to a feature branch prior to merge. This feature branch will be merged via #36404

kindsun added 2 commits March 11, 2019 11:04

Add file upload x-pack plugin

59c682d

Clean up

9abbc67

kindsun added enhancement New value added to drive a business result v8.0.0 v7.2.0 labels Mar 11, 2019

kobelb self-requested a review March 11, 2019 23:05

kindsun added the WIP Work in progress label Mar 11, 2019

Remove unneeded cluster config

13c187a

kindsun added 2 commits March 12, 2019 16:43

Remove unneeded test

bd921a7

First pass basic telemetry (not connected).

4445658

kobelb reviewed Mar 13, 2019

View reviewed changes

kindsun added 2 commits March 13, 2019 15:33

Basic telemetry connected

be3ab95

Review feedback

85c74bc

kindsun added 6 commits March 18, 2019 15:39

Merge remote-tracking branch 'upstream/master' into plugin-file-upload

a36e7e0

Revise telemetry to use savedObjectRepository. Capture metrics on app…

6015604

… and file types

Lots of cleanup, consolidation of logic

6b222f6

Merge remote-tracking branch 'upstream/master' into plugin-file-upload

dd577f3

Clean up, reorg

2aae63e

Update telem tests and telem functions

8de9a97

Add back import data model

63ec74e

kindsun mentioned this pull request Apr 8, 2019

[Maps] Geojson upload for maps app #34748

Closed

21 tasks

Merge remote-tracking branch 'upstream/master' into plugin-file-upload

4b4f85a

kindsun mentioned this pull request Apr 30, 2019

[File upload] Create developer documentation for file_upload plugin #35831

Open

Clean up and update telemetry tests

60a8a7c

kindsun added 2 commits May 1, 2019 14:48

Fix telemetry test issues and update corresponding code

e264ec1

Up chunk limit to 30 MB

377a3a1

Add file upload telemetry to saved objects management builder

3e48977

kindsun force-pushed the plugin-file-upload branch from 5f49bc9 to 3e48977 Compare May 1, 2019 23:40

Missing space

23c33ff

peteharverson mentioned this pull request May 2, 2019

[ML] Switch file data visualizer to use new file upload plugin for data import #35947

Closed

4 tasks

walterra reviewed May 2, 2019

View reviewed changes

Add descriptive comments to dynamic keys in telemetry fields

4361f88

thomasneirynck added the [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation label May 3, 2019

Merge remote-tracking branch 'upstream/master' into plugin-file-upload

1fba1d0

# Conflicts: # x-pack/index.js

kindsun changed the title ~~Add file upload x-pack plugin~~ [File upload] Add file upload x-pack plugin May 7, 2019

jgowdyelastic approved these changes May 8, 2019

View reviewed changes

walterra approved these changes May 8, 2019

View reviewed changes

kindsun mentioned this pull request May 9, 2019

[File upload] New plugin: file upload #36404

Merged

kindsun closed this May 10, 2019



		function generateId() {
		return Math.random().toString(36).substr(2, 9);

[File upload] Add file upload x-pack plugin #32945

[File upload] Add file upload x-pack plugin #32945

Conversation

kindsun commented Mar 11, 2019 • edited Loading

elasticmachine commented Mar 11, 2019

💚 Build Succeeded

elasticmachine commented Mar 12, 2019

💔 Build Failed

elasticmachine commented Mar 12, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 14, 2019

💔 Build Failed

elasticmachine commented Mar 26, 2019

💔 Build Failed

elasticmachine commented Mar 26, 2019

💔 Build Failed

kindsun commented May 1, 2019

elasticmachine commented May 1, 2019

💔 Build Failed

elasticmachine commented May 1, 2019

💔 Build Failed

elasticmachine commented May 2, 2019

💔 Build Failed

elasticmachine commented May 2, 2019

💚 Build Succeeded

peteharverson commented May 2, 2019

walterra left a comment

Choose a reason for hiding this comment

walterra May 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 2, 2019

💔 Build Failed

kindsun commented May 2, 2019

elasticmachine commented May 2, 2019

💔 Build Failed

elasticmachine commented May 3, 2019

elasticmachine commented May 7, 2019

💔 Build Failed

kindsun commented May 7, 2019

elasticmachine commented May 7, 2019

💚 Build Succeeded

kindsun commented May 7, 2019 • edited Loading

jgowdyelastic left a comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

kindsun commented May 10, 2019

kindsun commented Mar 11, 2019 •

edited

Loading

walterra May 2, 2019 •

edited

Loading

kindsun commented May 7, 2019 •

edited

Loading