-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
gh-135953: Add flamegraph reporter to sampling profiler #138304
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
Conversation
689b05c to
0803b28
Compare
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.
Brief review of the JS code. Please disregard any feedback that is irrelevant due to use of the D3 framework -- I am primarily used to working with vanilla/framework-less JS.
On formatting, obviously PEP 8 doesn't apply as it's not Python code, but it would still be good to keep to the spirit of it, and wrap at 79 characters or thereabouts for readability.
I've had advice/reviews in the past that from a security perspective it's best to avoid constructing HTML in JS directly and instead use e.g. document.createElement(). I don't know enough about the D3/JS interactions to know if this applies here, though. There was one place that looked like it was performing HTML escaping in JavaScript, which seems like something we might consider doing in the Python layer before the data gets to JS?
A
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.
Potentially use the SVG instead? https://github.com/python/devguide/blob/main/_static/python-logo.svg?short_path=f69d24a
The SVG is ~18x smaller, but it hasn't been minified.
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.
Perhaps add .min.js to the list of generated files in .gitattributes?
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.
Is it possible to apply these styles via a class, instead of inline?
Lib/profiling/sampling/flamegraph.js
Outdated
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.
Lib/profiling/sampling/flamegraph.js
Outdated
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.
Likewise, is it possible to use a class instead of inline styles?
Should we escape the HTML (line.replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">")) whilst we're still in Python?
For the color codes, could we use CSS variables so that if we change them in the future, we only need to change it in one place?
Lib/profiling/sampling/flamegraph.js
Outdated
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.
Perhaps extract to local variables instead of nested substitutions?
Lib/profiling/sampling/flamegraph.js
Outdated
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.
My understanding is that DOMContentLoaded may fire before the script loads, so the suggested pattern is to check for document.readyState. I don't know if this applies here or not, though.
Lib/profiling/sampling/flamegraph.js
Outdated
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.
Could this block be extracted to a helper function?
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.
Should we use importlib.resources here? I believe e.g. for the Windows embedable builds, the stdlib is distributed as a zip file so modules might not have a __file__ attribute.
I've had advice/reviews in the past that from a security perspective it's best to avoid constructing HTML in JS directly and instead use e.g. document.createElement(). I don't know enough about the D3/JS interactions to know if this applies here, though. There was one place that looked like it was performing HTML escaping in JavaScript, which seems like something we might consider doing in the Python layer before the data gets to JS?
Since this is a static template there are no advantages of doing this and having the web parts in Python is a bit messy specially since we cannot use something like Jinja. Having all the web parts in JS or the template itself is a common practice for this case and keeps the Python code focused just on the data part and not in creating HTML. Also since this is just a static file served with no dependencies so there are no concerns aboutsecurity or injection so I prefer to maximize maintainability.
I will give it a pass with prettier to format it as you indicate :)
Thanks for the review @AA-Turner. I think I addressed everything except for the comments around constructing HTML in JS. I also ended up breaking up the JS functions a bit in 7c4d792 as it was getting really unwieldy and I think this will make maintenance easier.
Uh oh!
There was an error while loading. Please reload this page.
Add
--flamegraphoption to the sampling profiler to produce a self-contained HTML report.