Log endpoint uses multipart form #1228

jameskerr · 2020-11-18T21:38:40Z

UPDATE:

I've added a new method to the fetcher called "upload". It uses XHR to make the request and track progress events. We still return an iterable stream of payloads which now include a message that looks like {type: "UploadProgress", progress: number}. This allows us to keep the progress bar unchanged as it was.

Fixes #1094

This changes the log post method in the zealot client to make a multipart form file upload, instead of sending the paths of the files. The pcaps post endpoint has remained unchanged.

It is not easy to get a progress bar for this type of request (it's impossible ATM using the Fetch API, we'd have to rewrite that method with XHR). I think we didn't have progress for log uploads anyway.

@philrz when you verify this, upload a large log file (or set of log files) over 4 GB. I expect it to work but that would be good to check.

P.S.
@mattnibs I noticed that the key for the warnings was a "nil". That may be a bug, see below:

alfred-landrum · 2020-11-19T00:30:08Z

@jameskerr : I know we can't get the same status messages that were used to implement the existing progress bar. I thought we would watch how much data has been uploaded to the backend api, and use that as the basis for a progress bar in this case? That's how the zapi command is giving a progressive status for the new api.

jameskerr · 2020-11-19T00:34:15Z

@alfred-landrum I understand, it's just that the zealot client is setup to use the modern, promise-based "Fetch" web api for making requests. The Fetch API has not created a way to hook into the upload progress events. We'd have to use the older XMLHttpRequest syntax for just this one api call. Definitely possible, but not quite plug and play.

https://usefulangle.com/post/321/javascript-fetch-upload-progress

philrz · 2020-11-19T00:36:42Z

Just to chime in with a visual, here's proof of the progress bar we've previously had for log import:

Regarding large imports, I'll do a final verification when this merges to master, but I did just do a quick test with the "wrccdc-year1" data set that's 12 GB of uncompressed Zeek TSV logs and they did import without complaint.

jameskerr · 2020-11-19T00:55:53Z

@philrz Thanks for your help! Glad to hear it worked with a big one. So that progress bar is there, but they are unable to search for anything until it completes. That's right.

mason-fish

Looks good! I had thought we could keep track of file upload as well, thanks for pointing out that constraint. Hopefully they do add it in to the fetch api soon.

My comments are mostly superficial, but pls take a look before merging.

mason-fish · 2020-11-19T03:51:07Z

src/js/flows/ingestFiles.ts

@@ -169,6 +171,13 @@ const trackProgress = (client, gDispatch, clusterId) => {
            gDispatch(space.setIngestProgress(logPostStatusToPercent(status)))
            updateSpaceDetails()
            break
+          case "LogPostResponse":
+            console.log(status)


left a straggler

mason-fish · 2020-11-19T03:51:24Z

src/js/flows/ingestFiles.ts

+          case "LogPostResponse":
+            console.log(status)
+            updateSpaceDetails()
+            ;(status.warnings || []).forEach((warning) => {


I think this semi-colon isn't doing anything. Also, would recommend using lodash's forEach for this instead

Yeah, prettier added that in. I guess when a line starts with parens, it needs the semi-colon.

mason-fish · 2020-11-19T03:55:54Z

src/js/brim/ingest/fileList.ts

@@ -1,46 +1,52 @@
 import {IngestFileType} from "./detectFileType"
 import lib from "../../lib"

-export type FileListData = {type: IngestFileType; path: string}[]
+export type FileListData = {type: IngestFileType; file: File}[]


not sure if you considered extending the File interface by annotating it with the new type field? would save all the extra .file.s since it would be flattened, just a thought!

Yeah true, I usually try to stay away from extending things like that. "Compose rather than inherit" mindset I guess. Is this something that is done conventionally in go?

I think the composition vs inheritance dilemma is the same in go. I do agree with the compose > inheritance mindset, but thought that in this case with just these two types, and one always depending on the other, extending would grant the small bonus of less typing as you access the imbedded File interface. I have no strong feelings in this case at all though!

mason-fish · 2020-11-24T19:58:50Z

zealot/src/fetcher/fetcher.ts

+        const iterator = createPushableIterator<ZealotPayload>()
+        const xhr = new XMLHttpRequest()
+
+        xhr.upload.addEventListener("progress", (e) => {


Alas, Deno does not implement XMLHttpRequest. And node does not implement FormData. So we run this test here in JSDOM. I tried to make it return upload progress events, but failed. This is good enough for jazz.

jameskerr requested review from mason-fish and mattnibs November 18, 2020 21:38

mason-fish approved these changes Nov 19, 2020

View reviewed changes

jameskerr force-pushed the multipart branch from 332f3b5 to 4de23e3 Compare November 20, 2020 21:16

jameskerr requested a review from mason-fish November 20, 2020 22:20

jameskerr added 3 commits November 24, 2020 11:58

Log endpoint uses multipart form

c19d44e

Update zealot tests

7f09529

Make jest root src

faf8f35

mason-fish reviewed Nov 24, 2020

View reviewed changes

jameskerr added 8 commits November 24, 2020 11:58

Added UploadProgress to ZealotPayloads type

051fc79

Extract parse method and NEW_LINE const

2321212

Create an iterator that can have values pushed into it

f793636

Add an upload method to the fetcher

f390600

Test for upload method

82faa76

Alas, Deno does not implement XMLHttpRequest. And node does not implement FormData. So we run this test here in JSDOM. I tried to make it return upload progress events, but failed. This is good enough for jazz.

Use the UploadProgress in the ingest file flow

a10c4aa

Fix type error

16097cd

Check warnings before iterating

a8dcc03

jameskerr force-pushed the multipart branch from 7b4a4da to a8dcc03 Compare November 24, 2020 19:59

jameskerr merged commit 33b3a15 into master Nov 24, 2020

jameskerr deleted the multipart branch November 24, 2020 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log endpoint uses multipart form #1228

Log endpoint uses multipart form #1228

jameskerr commented Nov 18, 2020 •

edited

Loading

alfred-landrum commented Nov 19, 2020 •

edited by philrz

Loading

jameskerr commented Nov 19, 2020 •

edited

Loading

philrz commented Nov 19, 2020

jameskerr commented Nov 19, 2020

mason-fish left a comment

mason-fish Nov 19, 2020

jameskerr Nov 20, 2020

mason-fish Nov 19, 2020

jameskerr Nov 20, 2020 •

edited

Loading

mason-fish Nov 19, 2020

jameskerr Nov 20, 2020

mason-fish Nov 24, 2020

mason-fish Nov 24, 2020

Log endpoint uses multipart form #1228

Log endpoint uses multipart form #1228

Conversation

jameskerr commented Nov 18, 2020 • edited Loading

alfred-landrum commented Nov 19, 2020 • edited by philrz Loading

jameskerr commented Nov 19, 2020 • edited Loading

philrz commented Nov 19, 2020

jameskerr commented Nov 19, 2020

mason-fish left a comment

Choose a reason for hiding this comment

mason-fish Nov 19, 2020

Choose a reason for hiding this comment

jameskerr Nov 20, 2020

Choose a reason for hiding this comment

mason-fish Nov 19, 2020

Choose a reason for hiding this comment

jameskerr Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

mason-fish Nov 19, 2020

Choose a reason for hiding this comment

jameskerr Nov 20, 2020

Choose a reason for hiding this comment

mason-fish Nov 24, 2020

Choose a reason for hiding this comment

mason-fish Nov 24, 2020

Choose a reason for hiding this comment

jameskerr commented Nov 18, 2020 •

edited

Loading

alfred-landrum commented Nov 19, 2020 •

edited by philrz

Loading

jameskerr commented Nov 19, 2020 •

edited

Loading

jameskerr Nov 20, 2020 •

edited

Loading