-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fixes #2073 -- Added DatabaseStore for persistent debug data storage. #2121
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
base: serializable
Are you sure you want to change the base?
Changes from all commits
6b54dca
17b27ce
a020331
4e71a4a
d37468f
677a80a
9d25201
4aadebe
fff3fae
d3329d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
initial = True | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name="HistoryEntry", | ||
fields=[ | ||
( | ||
"request_id", | ||
models.UUIDField(primary_key=True, serialize=False), | ||
), | ||
("data", models.JSONField(default=dict)), | ||
("created_at", models.DateTimeField(auto_now_add=True)), | ||
], | ||
options={ | ||
"verbose_name": "history entry", | ||
"verbose_name_plural": "history entries", | ||
"ordering": ["-created_at"], | ||
}, | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
from django.db import models | ||
from django.utils.translation import gettext_lazy as _ | ||
|
||
|
||
class HistoryEntry(models.Model): | ||
request_id = models.UUIDField(primary_key=True) | ||
data = models.JSONField(default=dict) | ||
created_at = models.DateTimeField(auto_now_add=True) | ||
|
||
class Meta: | ||
verbose_name = _("history entry") | ||
verbose_name_plural = _("history entries") | ||
ordering = ["-created_at"] | ||
|
||
def __str__(self): | ||
return str(self.request_id) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,10 +6,12 @@ | |
from typing import Any | ||
|
||
from django.core.serializers.json import DjangoJSONEncoder | ||
from django.db import transaction | ||
from django.utils.encoding import force_str | ||
from django.utils.module_loading import import_string | ||
|
||
from debug_toolbar import settings as dt_settings | ||
from debug_toolbar.models import HistoryEntry | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
@@ -140,5 +142,89 @@ def panels(cls, request_id: str) -> Any: | |
yield panel, deserialize(data) | ||
|
||
|
||
class DatabaseStore(BaseStore): | ||
@classmethod | ||
def _cleanup_old_entries(cls): | ||
""" | ||
Enforce the cache size limit - keeping only the most recently used entries | ||
up to RESULTS_CACHE_SIZE. | ||
""" | ||
# Determine which entries to keep | ||
keep_ids = cls.request_ids() | ||
|
||
# Delete all entries not in the keep list | ||
if keep_ids: | ||
HistoryEntry.objects.exclude(request_id__in=keep_ids).delete() | ||
|
||
@classmethod | ||
def request_ids(cls): | ||
"""Return all stored request ids within the cache size limit""" | ||
cache_size = dt_settings.get_config()["RESULTS_CACHE_SIZE"] | ||
return list( | ||
HistoryEntry.objects.all()[:cache_size].values_list("request_id", flat=True) | ||
) | ||
|
||
@classmethod | ||
def exists(cls, request_id: str) -> bool: | ||
"""Check if the given request_id exists in the store""" | ||
return HistoryEntry.objects.filter(request_id=request_id).exists() | ||
|
||
@classmethod | ||
def set(cls, request_id: str): | ||
"""Set a request_id in the store and clean up old entries""" | ||
with transaction.atomic(): | ||
# Create the entry if it doesn't exist (ignore otherwise) | ||
_, created = HistoryEntry.objects.get_or_create(request_id=request_id) | ||
|
||
# Only enforce cache size limit when new entries are created | ||
if created: | ||
cls._cleanup_old_entries() | ||
|
||
@classmethod | ||
def clear(cls): | ||
"""Remove all requests from the store""" | ||
HistoryEntry.objects.all().delete() | ||
|
||
@classmethod | ||
def delete(cls, request_id: str): | ||
"""Delete the stored request for the given request_id""" | ||
HistoryEntry.objects.filter(request_id=request_id).delete() | ||
|
||
@classmethod | ||
def save_panel(cls, request_id: str, panel_id: str, data: Any = None): | ||
"""Save the panel data for the given request_id""" | ||
# First ensure older entries are cleared if we exceed cache size | ||
cls.set(request_id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder what the The call can probably be removed here because we're immediately doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll push the rest of changes for now, while this is being discussed. For reference, here's our the @classmethod
def save_panel(cls, request_id: str, panel_id: str, data: Any = None):
"""Save the panel data for the given request_id"""
cls.set(request_id)
cls._request_store[request_id][panel_id] = serialize(data) We can also call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
with transaction.atomic(): | ||
obj, _ = HistoryEntry.objects.get_or_create(request_id=request_id) | ||
store_data = obj.data | ||
store_data[panel_id] = serialize(data) | ||
obj.data = store_data | ||
obj.save() | ||
|
||
@classmethod | ||
def panel(cls, request_id: str, panel_id: str) -> Any: | ||
"""Fetch the panel data for the given request_id""" | ||
try: | ||
data = HistoryEntry.objects.get(request_id=request_id).data | ||
panel_data = data.get(panel_id) | ||
if panel_data is None: | ||
return {} | ||
return deserialize(panel_data) | ||
except HistoryEntry.DoesNotExist: | ||
return {} | ||
|
||
@classmethod | ||
def panels(cls, request_id: str) -> Any: | ||
"""Fetch all panel data for the given request_id""" | ||
try: | ||
data = HistoryEntry.objects.get(request_id=request_id).data | ||
for panel_id, panel_data in data.items(): | ||
yield panel_id, deserialize(panel_data) | ||
except HistoryEntry.DoesNotExist: | ||
return {} | ||
|
||
|
||
def get_store() -> BaseStore: | ||
return import_string(dt_settings.get_config()["TOOLBAR_STORE_CLASS"]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import uuid | ||
|
||
from django.test import TestCase | ||
|
||
from debug_toolbar.models import HistoryEntry | ||
|
||
|
||
class HistoryEntryTestCase(TestCase): | ||
def test_str_method(self): | ||
test_uuid = uuid.uuid4() | ||
entry = HistoryEntry(request_id=test_uuid) | ||
self.assertEqual(str(entry), str(test_uuid)) | ||
|
||
def test_data_field_default(self): | ||
"""Test that the data field defaults to an empty dict""" | ||
entry = HistoryEntry(request_id=uuid.uuid4()) | ||
self.assertEqual(entry.data, {}) | ||
|
||
def test_model_persistence(self): | ||
"""Test saving and retrieving a model instance""" | ||
test_uuid = uuid.uuid4() | ||
entry = HistoryEntry(request_id=test_uuid, data={"test": True}) | ||
entry.save() | ||
|
||
# Retrieve from database and verify | ||
saved_entry = HistoryEntry.objects.get(request_id=test_uuid) | ||
self.assertEqual(saved_entry.data, {"test": True}) | ||
self.assertEqual(str(saved_entry), str(test_uuid)) | ||
|
||
def test_default_ordering(self): | ||
"""Test that the default ordering is by created_at in descending order""" | ||
self.assertEqual(HistoryEntry._meta.ordering, ["-created_at"]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,7 @@ deps = | |
pygments | ||
selenium>=4.8.0 | ||
sqlparse | ||
django-csp | ||
django-csp<4.0 | ||
passenv= | ||
CI | ||
COVERAGE_ARGS | ||
|
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.
Similar to
set()
, do we want to usetransaction.atomic()
here as well?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.
This comment is showing under panel()... I wonder if somehow the comment shifted and it was meant for save_panel().
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 meant this comment for both methods.
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.
As per our call, there's a concern about panels updating this concurrently and needing to avoid clearing another panel's data when saving.