-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
JSON renderer #46
JSON renderer #46
Conversation
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 iddan - I think the general output format is the right approach.
Could you add at least one test that reads the output of this renderer and checks there are the expected functions there?
pyinstrument/renderers.py
Outdated
def _json_frame(frame): | ||
return { | ||
'function': frame.function, | ||
'file_path': frame.file_path_short, |
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.
In the interests of creating a generally useful solution, I'd rather this was called file_path_short, in case we want to add file_path in the future.
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.
Cool
pyinstrument/renderers.py
Outdated
@@ -128,6 +129,21 @@ def render_frame(self, frame): | |||
result += '</div></div>' | |||
|
|||
return result | |||
|
|||
def _json_frame(frame): |
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.
Can this be a method on the JSONRenderer object?
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.
No problem
pyinstrument/renderers.py
Outdated
preferred_recorder = 'time_aggregating' | ||
|
||
def render(self, frame): | ||
return json.dumps(_json_frame(frame)) |
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.
might be nice to add indent=2
to make the output prettier on the terminal
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 though about it: many outputs can generate big outputs so adding spacing chars would increase their size significantly. Maybe this should be another renderer / CLI option?
If you go the UNIX way you can always do: python -m pyinstrument script.py | python -m json.tool
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.
Hmm. I don't agree. JSON is a really verbose format. All those repeated key names! All those numbers written out as strings.
The big advantage of JSON is human-readability - you can look at it and quickly understand it. To strip indentation is to lose that.
If size becomes a meaningful issue the right answer would be to remove frames with tiny durations, or use something like Protobuf.
(Re. piping through python -m json.tool
- it's a good idea, but I don't think the burden of a good user experience should be on the user)
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.
Okay, will add
Issued the comments |
Here is the app expecting the result: https://python-flame-chart.netlify.com |
Just waiting on the |
Done! |
Cheers @iddan ! |
Please let me know when it is released |
Apologies for the delay - working on a few more changes and tests before the next release. In the meantime, you can install from the master branch directly - |
Released as 2.2.1! |
Issues #44