-
Notifications
You must be signed in to change notification settings - Fork 25
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
Bug in live version, cannot load multiple files by URL #54
Comments
Qvis can't handle the above example because the files are hosted on localhost. Loading files using option 4 requires backend support. The addition of the The backend additionally merges the provided qlogs into one qlog containing the traces from the respective qlogs. I don't' think this is problematic in itself, but Qvis has no clear way of differentiating between the provided qlogs.
A possible solution (the easiest one probably) would be to move the backend looping behaviour to the frontend and to perform a backend fetch request for each individual file if it can't be downloaded via de fetch API. This would allow local hosted files to be uploaded via option 4 and it would also solve the differentiation issue mentioned above at the cost of a couple more requests. I do, however, not know how the |
I guess, I'm experiencing the same issue, when I try to load file not hosted on I'm trying to load the two qlog files:
This is the error message shown in qvis: |
…to qvis, more design foo See <quiclog/qvis#54 (comment)> for broken qvis links
So, this is a gift that keeps on giving... There are several issues in play at the same time here. @sedrubal's problem was that adding "etc." produces a 404 instead of CORS error, which caused the fallback to the backend server to fail. However, I couldn't just remove the "etc." as @MikeV-1642883 suggested, because that would lead to just the "file1" being downloaded, and the rest ignored if that single file was directly downloadable (I've added comments to describe this). This part is now fixed, but the next problem is that the files @sedrubal's trying to load are in the NDJSON format, which is currently not supported by the qvis backend, only the frontend... so the files now properly get sent to the backend, but it borks on the "malformed JSON". Since the NDJSON approach will be changed in the near future, I won't be adding that to the backend anytime soon. It should work though if you load qlog files not in NDJSON format (e.g., from aioquic instead of quic-go). This fix doesn't do anything for @MikeV-1642883's multiple-localhost-files problem, which is more complex. To do this without backend fallback, I'd have to fully directly load multiple files in the frontend, and then also have logic on what to do if say 1 out of 5 files fails to download, if there is no CORS enabled on one of the files, etc. This is why I hadn't really solved this issue yet... The commit above is quite dirty because I didn't have much time to properly refactor this. It's clear this entire setup needs a good overhaul sometime, but sadly I don't have time to do this atm. |
Thanks @rmarx for figuring that out 😉 |
Describe the bug
Trying to load multiple files by URL results in an error.
Format 1 (
?file=x.qlog
) works like a charm, but when using format 4 (?file1=x.qlog&file2=y.qlog&file3=z.qlog
) an error is thrown. The error also occurs when only trying to load a single file.The error seems to be a bug.
Meanwhile, the inability to load multiple files seems to be a lacking feature. I quickly skimmed though the code, and it seems like only one file is attempted to be fetched, the others are ignored.
To Reproduce
?file1=http://localhost:5000/qlog/f49d5b63efe7abca.qlog
ERROR loading URL
Expected behaviour
When only supplying
file1
: same behaviour as format 1, correctly loading the file.When supplying multiple files (
file1
,file2
, ...): correctly loading all supplied files.Error message
Note that the response has
%20etc.
appended to the URL, which is absent infile1
.Possible fix
The error can be fixed using the following patch, which works for a local build:
The text was updated successfully, but these errors were encountered: