Add Option to configure renderers in Django settings #158
Conversation
pyinstrument/middleware.py
Outdated
| """Return the renderer instance and output file extension. | ||
| """ | ||
| if path: | ||
| renderer = import_string(path)() |
There was a problem hiding this comment.
should we catch ImportError when the renderer is missing?
There was a problem hiding this comment.
No, ImportError seems reasonable for a misconfiguration I think.
pyinstrument/middleware.py
Outdated
| """Return the renderer instance and output file extension. | ||
| """ | ||
| if path: | ||
| renderer = import_string(path)() |
There was a problem hiding this comment.
No, ImportError seems reasonable for a misconfiguration I think.
pyinstrument/middleware.py
Outdated
| return renderer, "html" | ||
| elif isinstance(renderer, JSONRenderer): | ||
| return renderer, "json" | ||
| # should other renderers be discouraged? |
There was a problem hiding this comment.
Probably not discouraged, if we implement the class variable approach above.
pyinstrument/middleware.py
Outdated
| if isinstance(renderer, HTMLRenderer): | ||
| return renderer, "html" | ||
| elif isinstance(renderer, JSONRenderer): | ||
| return renderer, "json" |
There was a problem hiding this comment.
These extension values could be accessed through the Renderer object. E.g. a class variable on each could specify output_file_extension. That would allow custom renderers to participate.
pyinstrument/middleware.py
Outdated
| @@ -41,8 +57,10 @@ def process_response(self, request, response): | |||
| if hasattr(request, "profiler"): | |||
| profile_session = request.profiler.stop() | |||
|
|
|||
| renderer = HTMLRenderer() | |||
| output_html = renderer.render(profile_session) | |||
| configured_renderer = getattr(settings, "PYINSTRUMENT_RENDERER", None) | |||
There was a problem hiding this comment.
So... this is only the configured renderer for the profile_dir. Perhaps a better name for the option would be PYINSTRUMENT_PROFILE_DIR_RENDERER? Then also, we could move the rendering into the if profile_dir: section. That would save cycles rendering something that isn't used.
- Add output_file_extension attribute to the Renderer class - Use PYINSTRUMENT_PROFILE_DIR_RENDERER setting variable
| @@ -29,6 +29,10 @@ class Renderer: | |||
| Dictionary containing processor options, passed to each processor. | |||
| """ | |||
|
|
|||
| output_file_extension: str = "txt" | |||
There was a problem hiding this comment.
I have added the output_file_extension attribute.
| @@ -41,8 +61,10 @@ def process_response(self, request, response): | |||
| if hasattr(request, "profiler"): | |||
| profile_session = request.profiler.stop() | |||
|
|
|||
| renderer = HTMLRenderer() | |||
| output_html = renderer.render(profile_session) | |||
| configured_renderer = getattr(settings, "PYINSTRUMENT_PROFILE_DIR_RENDERER", None) | |||
There was a problem hiding this comment.
Keeping this here since this renderer is used in the if/else block to produce output.
joerick
added
the
automerge
| return renderer, renderer.output_file_extension | ||
| else: | ||
| print("Renderer should subclass: %s. Using HTMLRenderer" % Renderer) | ||
| return HTMLRenderer(), HTMLRenderer.output_file_extension |
There was a problem hiding this comment.
Pyright was pointing out this can return None. So when the renderer is not a subclass of Renderer, we will be using HTMLRenderer. Let me know if this is fine?
There was a problem hiding this comment.
I think it would be better to raise an error, instead. I'll update it.
There was a problem hiding this comment.
I've made the update. For some reason I wasn't able to push to this branch. I've pushed it to 69b195b - if you could cherry-pick that here, I think we're good to go.
joerick
removed
the
automerge
…] (#163) * Add Option to configure renderers in Django settings * Run precommit hook on all files * Incorporate the review feedback - Add output_file_extension attribute to the Renderer class - Use PYINSTRUMENT_PROFILE_DIR_RENDERER setting variable * Update pyinstrument/middleware.py * Remove exit condition and use html renderer when render is missing * Tidy up, raise error on misconfig * pre-commit fixes Co-authored-by: Kracekumar <me@kracekumar.com> Co-authored-by: pre-commit <pre-commit@example.com>
|
This was merged in #163 |
|
Thanks a lot! Couldn't do complete earlier. |
Description
Why?
Changes
PYINSTRUMENT_RENDERERwhich takes in the path of the renderer to use.json, HTML, txtfor now. I'm happy to make the extension configurable too.Let me know If I need to add test cases and modify the docs.
Thanks.