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

Support importing partial JSON files #202

Merged
merged 1 commit into from
Feb 9, 2019
Merged

Conversation

loganek
Copy link
Contributor

@loganek loganek commented Jan 27, 2019

Partial files are allowed in many specs, e.g. Trace Event Format,
so the viewer should be able to load partial files as well.

Partial files are allowed in many specs, e.g. Trace Event Format,
so the viewer should be able to load partial files as well.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 46.754% when pulling 325f0fa on loganek:master into ddc6130 on jlfwong:master.

@loganek
Copy link
Contributor Author

loganek commented Feb 4, 2019

Hey @jlfwong , could you please review that change? Thanks!

@jlfwong
Copy link
Owner

jlfwong commented Feb 4, 2019

@loganek Hi! My home internet has been down for the last week, but I'll try to get to this soon!

I'm generally wary of adding third party dependencies, but I took a look at partial-json-parser, and it looks like it has no dependencies and is enough code that I'd rather not just do it myself, so I'm happy to accept this.

I'll try to take a closer look later this week, merge if I don't find anything concerning, then do a release that picks this up.

Out of curiousity, were you motivated to do this because you have Chrome trace profiles that are not terminated? What was generating them?

@loganek
Copy link
Contributor Author

loganek commented Feb 4, 2019

@jlfwong So sorry, I didn't want to rush you at all! Just thought that you missed the PR, so wanted to ping you, but please take your time, it's not super urgent for me.

Yeah, I wasn't sure what's your opinion about third party dependencies, but since it wasn't very time consuming, I decided to give a try.

Yes, I did this PR exactly because my files are not terminated; those files are generated by hawktracer-converter (https://github.com/loganek/hawktracer-converter/) which is a client app of my profiler (https://github.com/amzn/hawktracer). I guess I could work-around this in the converter to close the file properly, but since the format specification allow to have non-terminated files, I thought it'd be more correct to fix your viewer instead.

Btw. great tool, it feels much faster than the one embedded to chrome, and flamegraphs is a nice feature too. Good job, and thanks for doing that :)

@jlfwong
Copy link
Owner

jlfwong commented Feb 9, 2019

This is great as is :) Thanks for taking the time to carefully craft this PR!

@jlfwong jlfwong merged commit cfc8fe8 into jlfwong:master Feb 9, 2019
jlfwong pushed a commit that referenced this pull request Feb 9, 2019
@jlfwong
Copy link
Owner

jlfwong commented Feb 9, 2019

@loganek Ah... sorry I spoke too soon. I had to revert the commit because partial-json-parser doesn't guarantee termination: gov-ind/partial-json-parser#3.

The repository now depends upon babel too as a non-dev dependency, so at this point I think I would seriously consider writing my own partial JSON parser to avoid taking on too many additional transitive dependencies.

The specific cases of partial json files supported by the Trace Event Format is also a small subset of what that module tries to support. For example, it doesn't support unterminated strings. In fact, as far as I can tell, the only allowance it specifies is this:

When provided as a string to the importer the ] at the end of the JSON Array Format is optional. The Trace Viewer importer will automatically add a ] if needed to turn the string into valid JSON.. This is to support tracing systems that can not cleanly finish writing the trace. For example, when tracing the exit of a program.

(From https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#).

So I think retrying import after appending a ] would be pretty straightforward.

jlfwong added a commit that referenced this pull request Feb 18, 2019
This PR introduces support for importing JSON based profiles that are missing a terminating `]` (and possibly have an extraneous `,`).

This is similar to #202, but takes a much more targeted and simple approach.

I'm confident that this approach is sufficient because this is exactly what `chrome://tracing` does: https://github.com/catapult-project/catapult/blob/27e047e0494df162022be6aa8a8862742a270232/tracing/tracing/extras/importer/trace_event_importer.html#L197-L208

Fixes #204
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 this pull request may close these issues.

3 participants