quiclog / qvis

QUIC and HTTP/3 visualization tools
https://qvis.edm.uhasselt.be
MIT License
172 stars 25 forks source link

Bug in live version, cannot load multiple files by URL #54

Open VandersandenMike opened 2 years ago

VandersandenMike commented 2 years ago

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

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

ConnectionStore.ts:302 ConnectionStore:LoadFilesFromServer : ERROR : trace not added to qvis! : {
file1: "http://localhost:5000/qlog/f49d5b63efe7abca.qlog"
}
Response: {
url: "http://localhost:5000/qlog/f49d5b63efe7abca.qlog%20etc."
}

Note that the response has %20etc. appended to the URL, which is absent in file1.

Possible fix The error can be fixed using the following patch, which works for a local build:

diff --git a/visualizations/src/store/ConnectionStore.ts b/visualizations/src/store/ConnectionStore.ts
index fbce71b..9d3606a 100644
--- a/visualizations/src/store/ConnectionStore.ts
+++ b/visualizations/src/store/ConnectionStore.ts
@@ -136,7 +136,7 @@ export default class ConnectionStore extends VuexModule {
             urlToLoad = queryParameters.list;
         }
         else if ( queryParameters.file1 ){
-            urlToLoad = queryParameters.file1 + " etc.";
+            urlToLoad = queryParameters.file1;
         }

         if ( urlToLoad === "" ){
JorisHerbots commented 2 years ago

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 %20etc. string actually makes the frontend download check fail. Perhaps not the most elegant way of doing this? 😛

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.

Two aioquic traces provided via option 4 Two aioquic traces provided via option 4

Two aioquic traces provided via option 1 The same two aioquic traces provided via option 1

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 list parameter behaves. If it also returns a singular qlog with merged traces, then maybe we should be looking for a better way of differentiating traces if they originate from different qlogs?

sedrubal commented 2 years ago

I guess, I'm experiencing the same issue, when I try to load file not hosted on localhost using this URL:

https://qvis.quictools.info/#/files?file1=https%3A%2F%2Finterop.sedrubal.de%2Flogs%2Flogs_sat_2021-09-12%2Fngtcp2_quic-go%2Fsat%2F1%2Fclient.qlog&file2=https%3A%2F%2Finterop.sedrubal.de%2Flogs%2Flogs_sat_2021-09-12%2Fngtcp2_quic-go%2Fsat%2F1%2Fserver.qlog

I'm trying to load the two qlog files:

This is the error message shown in qvis:

Screenshot_20210929_120406

rmarx commented 2 years ago

So, this is a gift that keeps on giving... There are several issues in play at the same time here. There is a partial fix in the commit above (and live on qvis.quictools.info), but I don't think this (fully) fixes things for either of you...

@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.

sedrubal commented 2 years ago

Thanks @rmarx for figuring that out 😉