Skip to content
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 viewer HTTP API does not support .eval format #1185

Closed
tadamcz opened this issue Jan 24, 2025 · 8 comments · Fixed by #1189
Closed

Log viewer HTTP API does not support .eval format #1185

tadamcz opened this issue Jan 24, 2025 · 8 comments · Fixed by #1189

Comments

@tadamcz
Copy link
Contributor

tadamcz commented Jan 24, 2025

It tries to parse the binary as JSON and raises:

Error: JSON5: invalid character 'P' at 1:1

The necessary changes are somewhere in api-http.mjs. (Note that I'm about to modify that file with #1184, so it would be best to make this change downstream from that.)

@dragonstyle
Copy link
Collaborator

It does support eval files (we use them quite a bit) so there is likely something specific in your scenario that isn't being handled properly by the viewer. Can you say more about how to reproduce this?

@tadamcz
Copy link
Contributor Author

tadamcz commented Jan 24, 2025

Sorry to be unclear. By the HTTP API of the viewer, I mean the stuff defined in api-http.mjs and index.mjs

I'm passing in a URL to an .eval file in the log_file URL query parameter:

const log_file = urlParams.get("log_file");

@dragonstyle
Copy link
Collaborator

I'm familiar with that code.

So you're just forming a url with a log file pointing to an eval file- I'll give that a try later today and report back.

@tadamcz
Copy link
Contributor Author

tadamcz commented Jan 24, 2025

It's calling asyncJsonParse

export const asyncJsonParse = async (text) => {
const encoder = new TextEncoder();
const encodedText = encoder.encode(text);
const blob = new Blob([kWorkerCode], { type: "application/javascript" });
const blobURL = URL.createObjectURL(blob);
const worker = new Worker(blobURL);
try {
const result = new Promise((resolve, reject) => {
worker.onmessage = function (e) {
if (e.data.success) {
resolve(e.data.result);
} else {
reject(new Error(e.data.error));
}
};
worker.onerror = function (error) {
reject(new Error(error.message));
};
});
worker.postMessage({ scriptContent: kJson5ScriptBase64, encodedText }, [
encodedText.buffer,
]);
return await result;
} finally {
worker.terminate();
URL.revokeObjectURL(blobURL);
}
};

and throwing on

reject(new Error(e.data.error));

because the contents are not JSON.

@tadamcz
Copy link
Contributor Author

tadamcz commented Jan 24, 2025

The fix might be as simple as using openRemoteLogFile

export const openRemoteLogFile = async (api, url, concurrency) => {

I'm happy to take a crack at it if you'd like. If you could review #1184 first that would be helpful, so that I don't have to deal with potential merge conflicts.

@dragonstyle
Copy link
Collaborator

Yeah I think the question is why it is improperly treating the file as a json file rather than and eval file.

@tadamcz
Copy link
Contributor Author

tadamcz commented Jan 24, 2025

Ah, so it's supposed to already detect .eval files when passed in? Can you show me where that happens?

dragonstyle added a commit that referenced this issue Jan 25, 2025
Since the apis aren’t really aware of file type differences, the log_file url param shouldn’t be used inside the API to resolve log_paths. Instead, resolve it at the client level, if neeeded.

Fixes #1185
@dragonstyle
Copy link
Collaborator

Reasoning about the log file based upon the type happens one layer up in the client-api. I've opened a PR which moves the file handling for this case up to that level (see referenced PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants