-
Notifications
You must be signed in to change notification settings - Fork 131
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
Log endpoint uses multipart form #1228
Conversation
@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 |
@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 |
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. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/js/flows/ingestFiles.ts
Outdated
@@ -169,6 +171,13 @@ const trackProgress = (client, gDispatch, clusterId) => { | |||
gDispatch(space.setIngestProgress(logPostStatusToPercent(status))) | |||
updateSpaceDetails() | |||
break | |||
case "LogPostResponse": | |||
console.log(status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a straggler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
src/js/flows/ingestFiles.ts
Outdated
case "LogPostResponse": | ||
console.log(status) | ||
updateSpaceDetails() | ||
;(status.warnings || []).forEach((warning) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this semi-colon isn't doing anything. Also, would recommend using lodash's forEach
for this instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, prettier added that in. I guess when a line starts with parens, it needs the semi-colon.
@@ -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}[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
const iterator = createPushableIterator<ZealotPayload>() | ||
const xhr = new XMLHttpRequest() | ||
|
||
xhr.upload.addEventListener("progress", (e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
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.
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: