Don't try to undo cache method monkey patching

[living180]

Description

Trying to undo the monkey patch of cache methods in the CachePanel.disable_instrumentation() method is fragile in the presence of other code which may also monkey patch the same methods (such as Sentry's Django integration), and there are theoretically situations where it is actually impossible to do correctly. Thus once a cache has been monkey-patched, leave it that way, and instead rely on checking in the patched methods to see if recording needs to happen. This is done via a _djdt_panel attribute which is set to the current panel in the enable_instrumentation() method and then set to None in the disable_instrumentation() method.

Fixes #1769

Checklist:

• I have added the relevant tests for this change.
• I have added an item to the Pending section of docs/changes.rst.

[matthiask]

I think it's a good idea. I haven't tested it out, it would certainly be helpful if the change were tested by those who reported crashes in the other issue.

Sorry for the bikeshedding, but maybe _djdt_recording was a more fitting name than _djdt_panel; or maybe I misunderstand the intention of the variable?

I also think we should generally move towards using from time import perf_counter instead of time.time() to measure short durations; my understanding is that time.time could be affected by updating the system clock. It's extremely unlikely to matter of course, but perf_counter would be more correct. And it's also what Django does, see https://github.com/django/django/blob/1586a09b7949bbb7b0d84cb74ce1cadc25cbb355/django/test/utils.py#L934-L941

[living180]

Sorry for the bikeshedding, but maybe _djdt_recording was a more fitting name than _djdt_panel; or maybe I misunderstand the intention of the variable?

So the type of the variable actually changed: previously it was a bool, now it is an instance of CachePanel if instrumentation is enabled, or None otherwise. This is necessary because with this PR, once a cache has been monkey patched, it stays that way, but each request creates a new CachePanel (previously the cache was being monkey patched for each request specifically for the associated CachePanel).

[living180]

I also think we should generally move towards using from time import perf_counter instead of time.time() to measure short durations; my understanding is that time.time could be affected by updating the system clock. It's extremely unlikely to matter of course, but perf_counter would be more correct. And it's also what Django does, see https://github.com/django/django/blob/1586a09b7949bbb7b0d84cb74ce1cadc25cbb355/django/test/utils.py#L934-L941

Totally agree, though that is really orthogonal to this change. I can look at making a new PR switching to perf_counter across the codebase (I'm pretty sure the SQL panel would also need to be updated, and perhaps others).

[matthiask]

So the type of the variable actually changed: previously it was a bool, now it is an instance of CachePanel if instrumentation is enabled, or None otherwise. This is necessary because with this PR, once a cache has been monkey patched, it stays that way, but each request creates a new CachePanel (previously the cache was being monkey patched for each request specifically for the associated CachePanel).

Ah of course, thank you!