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

Stack trace rework #1631

Merged
merged 5 commits into from
May 29, 2022
Merged

Conversation

living180
Copy link
Contributor

Add a new stack recording mechanism which includes caching on a per-request basis to reduce expensive file system operations. Update the cache and SQL panels to use this new mechanism to record stack traces.

The caching is implemented on top of asgiref.local.Local to ensure that requests being handled in different thread/async task contexts have independent caches.

This PR supersedes #1606.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code makes a lot of sense to me. Thanks!

This should make it possible to delete the old get_stack, tidy_stacktrace functionality? I don't think we should treat those utilities as API of django-debug-toolbar (which would mean that we'd have to deprecate them and keep them around for some time) but we should instead delete them here and now. This would improve the diffstat, too -- it shows mostly added lines until now.

I haven't checked out the code locally, the approval is based on reading the code and a passing testsuite.

@living180
Copy link
Contributor Author

This should make it possible to delete the old get_stack, tidy_stacktrace functionality?

I'm a -1 on deleting them. I know for certain that there are multiple third-party panels that use them, even though they aren't part of the documented API, and deleting them would break those panels. I'm happy to add deprecation warnings to them, however.

@living180
Copy link
Contributor Author

living180 commented May 29, 2022

And then a third-party panel that wanted to support multiple debug toolbar versions without triggering deprecation warnings could use a snippet like the following:

try:
    from debug_toolbar.utils import get_stack_trace
except ImportError:
    from debug_toolbar.utils import get_stack, tidy_stacktrace

    def get_stack_trace():
        return tidy_stacktrace(reversed(get_stack()))

@matthiask
Copy link
Member

I see.

The problem is that these functions do not seem to be called in any tests anymore and breakage/bit rot will happen sooner rather than later. Therefore we should definitely deprecate those functions.

I think it would be nice to add the snippet to the release notes and warn everyone that the old functionality will be removed in the release after the next. If you know of third party panels using these functions giving them a heads up would be extra nice. They could even help testing the relevant code :)

living180 added 5 commits May 29, 2022 19:06
* Add a private _StackTraceRecorder class to debug_toolbar.utils which
  implements caching to reduce the overhead of expensive file system
  operations.  This class has a get_stack_trace() method which combines
  the functionality of the get_stack() and tidy_stacktrace() functions.
* Add a new debug_toolbar.utils function, get_stack_trace() which gets
  or instantiates a thread/async task context-local _StackTraceRecorder
  instance and returns a stack trace using its get_stack_trace() method.
* Add a new debug_toolbar.utils function, clear_stack_trace_caches(),
  which removes any thread/async task context-local _StackTraceRecorder
  instance.
* Update the DebugToolbarMiddleware to call the
  clear_stack_trace_caches() function after processing a request to
  ensure that each subsequent request gets a clean cache.
@living180 living180 force-pushed the stacktrace_rework branch from 0f8875a to 1c586c9 Compare May 29, 2022 16:46
@living180
Copy link
Contributor Author

Just pushed an update that adds deprecation warnings to get_stack() and tidy_stacktrace(), and also adds a test to ensure they get run. Also added a note about the deprecation of these functions to the change log.

@matthiask matthiask merged commit be4e952 into django-commons:main May 29, 2022
@matthiask
Copy link
Member

Thanks! Let's move ahead with this.

@living180 living180 deleted the stacktrace_rework branch May 29, 2022 18:02
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.

2 participants