Api statistics #271
Api statistics #271
Conversation
WalkthroughAdds per-user API usage tracking: a new Django model (APIUsage) with migration, admin registration and UI display, a utility to collect recent usage, and view-layer session tracking (including streaming wrappers) that records transfer size, request counts, and last access times. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant APIView as API View
participant Track as track_session
participant DB as APIUsage DB
participant Stream as Stream Wrapper
participant Renderer as Response Renderer
participant AdminView as Admin Section View
participant Util as collect_latest_api_usage
participant Template as Admin Template
Note over Client, APIView: API request handling with tracking
Client->>APIView: HTTP request
APIView->>Track: enter track_session(user)
Track->>DB: get or create APIUsage for owner
DB-->>Track: session object
alt streaming response
APIView->>Stream: wrap streaming iterator/response
Stream->>Renderer: yield chunks
Renderer-->>Stream: chunk sizes
Stream->>Track: accumulate transfer size
else non-streaming
APIView->>Renderer: render full response
Renderer-->>Track: report response size
end
APIView->>Track: exit track_session -> finalize
Track->>DB: atomic update F(total_transfer_size += N, total_number_requests += 1), update time_last_accessed
DB-->>Track: updated
APIView-->>Client: return response
Note over AdminView, Template: Admin display latest usage
AdminView->>Util: collect_latest_api_usage()
Util->>DB: query last 3 APIUsage ordered desc
DB-->>Util: records
Util-->>AdminView: api_stats
AdminView->>Template: render admin_section with api_stats
Template->>Template: iterate and format table rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request Review: API StatisticsOverviewThis PR adds API usage tracking for DICOM Web endpoints, including request counts, transfer sizes, and timestamps. The implementation adds a new Code Quality & Best Practices✅ Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
adit/core/views.py (1)
34-34: Cross-app dependency introduced from core to dicom_web.This import creates a dependency from
adit.coretoadit.dicom_web. While this works, it inverts the typical dependency direction where feature apps depend on core, not vice versa. Consider movingcollect_latest_api_usageto a more neutral location or having the admin section dynamically discover available statistics collectors (similar tojob_stats_collectorspattern on line 37).adit/dicom_web/apps.py (1)
19-22: Add return type annotation for clarity.The function lacks a type hint for its return value.
-def collect_latest_api_usage(): +def collect_latest_api_usage() -> "QuerySet[APIUsage]": from .models import APIUsage return APIUsage.objects.order_by("-time_last_accessed")[:3]Alternatively, if you want to return a list instead of a sliced QuerySet:
from typing import TYPE_CHECKING if TYPE_CHECKING: from .models import APIUsage def collect_latest_api_usage() -> list["APIUsage"]: from .models import APIUsage return list(APIUsage.objects.order_by("-time_last_accessed")[:3])adit/core/templates/core/admin_section.html (1)
42-49: Consider renaming loop variable and adding empty state.The loop variable
sessionis misleading since the model isAPIUsage, not a session. Consider renaming tousageorapi_usagefor clarity.Also, consider adding an empty state message when there's no API usage data:
<tbody> - {% for session in api_stats %} + {% for usage in api_stats %} <tr> - <td>{{ session.owner }}</td> - <td>{{ session.time_last_accessed }}</td> - <td>{{ session.total_transfer_size | filesizeformat }}</td> - <td>{{ session.total_number_requests }}</td> + <td>{{ usage.owner }}</td> + <td>{{ usage.time_last_accessed }}</td> + <td>{{ usage.total_transfer_size | filesizeformat }}</td> + <td>{{ usage.total_number_requests }}</td> </tr> + {% empty %} + <tr> + <td colspan="4" class="text-muted text-center">No API usage data available</td> + </tr> {% endfor %} </tbody>adit/dicom_web/migrations/0004_apiusage.py (1)
16-44: Consider adding database index ontime_last_accessed.The
collect_latest_api_usage()function queriesORDER BY time_last_accessed DESC. As the table grows, this could benefit from an index.Add an index in the model's Meta class or via a subsequent migration:
class Meta: indexes = [ models.Index(fields=['-time_last_accessed']), ]adit/dicom_web/admin.py (1)
16-16: Reconsidertotal_number_requestsin list_filter.Filtering by exact request count values isn't typically useful. Consider removing it or using a custom range-based filter if needed.
- list_filter = ("total_number_requests", "owner") + list_filter = ("owner",)adit/dicom_web/models.py (1)
18-26: Clarify if APIUsage should be one-per-user.The views use
get_or_create(owner=user)suggesting oneAPIUsagerecord per user, but there's nounique=Trueconstraint onowner. Consider adding a unique constraint to enforce this at the database level:class APIUsage(models.Model): time_last_accessed = models.DateTimeField(default=timezone.now) total_transfer_size = models.BigIntegerField(default=0) total_number_requests = models.IntegerField(default=0) owner = models.ForeignKey( settings.AUTH_USER_MODEL, on_delete=models.CASCADE, - related_name="api_usage", + related_name="api_usage", + unique=True, )Or use
OneToOneFieldif the intent is strictly one record per user:owner = models.OneToOneField( settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="api_usage", )adit/dicom_web/views.py (2)
73-80: RedundantNonecheck fortransfer_size.The parameter
transfer_sizehas a default value of0, so it can never beNoneunless explicitly passed as such (which doesn't happen in the codebase).@database_sync_to_async def _finalize_statistic(self, statistic: APIUsage, transfer_size: int = 0) -> None: - if transfer_size is not None: - APIUsage.objects.filter(pk=statistic.pk).update( - total_transfer_size=F("total_transfer_size") + transfer_size, - total_number_requests=F("total_number_requests") + 1, - time_last_accessed=timezone.now(), - ) + APIUsage.objects.filter(pk=statistic.pk).update( + total_transfer_size=F("total_transfer_size") + transfer_size, + total_number_requests=F("total_number_requests") + 1, + time_last_accessed=timezone.now(), + )
318-318: Replacegetattrwith direct attribute access.Per static analysis:
getattr(request, "accepted_renderer")with a constant string is equivalent torequest.accepted_rendererbut less readable. This pattern repeats on lines 355, 383, 421, 455, and 499.- renderer = cast(DicomWebWadoRenderer, getattr(request, "accepted_renderer")) + renderer = cast(DicomWebWadoRenderer, request.accepted_renderer)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
adit/core/templates/core/admin_section.html(1 hunks)adit/core/views.py(2 hunks)adit/dicom_web/admin.py(1 hunks)adit/dicom_web/apps.py(1 hunks)adit/dicom_web/migrations/0004_apiusage.py(1 hunks)adit/dicom_web/models.py(2 hunks)adit/dicom_web/views.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
adit/core/views.py (1)
adit/dicom_web/apps.py (1)
collect_latest_api_usage(19-22)
adit/dicom_web/apps.py (1)
adit/dicom_web/models.py (1)
APIUsage(18-29)
adit/dicom_web/admin.py (1)
adit/dicom_web/models.py (2)
APIUsage(18-29)DicomWebSettings(8-15)
🪛 Ruff (0.14.7)
adit/dicom_web/migrations/0004_apiusage.py
11-14: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
16-45: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
adit/dicom_web/views.py
94-94: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
318-318: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
355-355: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
383-383: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
421-421: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
455-455: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
499-499: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci
- GitHub Check: claude-review
🔇 Additional comments (4)
adit/core/views.py (1)
47-55: LGTM - API stats integration into admin section.The integration follows the existing pattern for
job_stats. The query is lightweight (3 records max).adit/dicom_web/views.py (3)
277-292: LGTM - Streaming wrapper properly tracks transfer size.The
_StreamingSessionWrappercorrectly accumulates chunk sizes and finalizes statistics in thefinallyblock, ensuring stats are recorded even if streaming is interrupted.
129-191: LGTM - Query views correctly track transfer size.The query views consistently use
len(json.dumps(response_data).encode())to calculate transfer size before returning the response.
295-324: LGTM - Streaming retrieve views use wrapper correctly.The streaming retrieve views properly wrap the renderer output with
_StreamingSessionWrapperto track transfer size during streaming.
| def get_size(self, obj): | ||
| size = obj.total_transfer_size | ||
| for unit in ["B", "KB", "MB", "GB", "TB"]: | ||
| if size < 1024: | ||
| return f"{size:.2f} {unit}" | ||
| size /= 1024 | ||
|
|
||
| get_size.short_description = "Total Transfer Size" |
There was a problem hiding this comment.
get_size doesn't return a value for sizes >= 1024 TB.
If total_transfer_size exceeds 1024 TB, the loop completes without returning, causing None to be displayed. Also consider using Django's filesizeformat for consistency with the template.
+from django.template.defaultfilters import filesizeformat
+
+class APIUsageAdmin(admin.ModelAdmin):
+ # ...
+
def get_size(self, obj):
- size = obj.total_transfer_size
- for unit in ["B", "KB", "MB", "GB", "TB"]:
- if size < 1024:
- return f"{size:.2f} {unit}"
- size /= 1024
+ return filesizeformat(obj.total_transfer_size)
get_size.short_description = "Total Transfer Size"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In adit/dicom_web/admin.py around lines 25 to 32, get_size currently loops
through units and returns nothing when size is >= 1024 TB; update it to always
return a formatted string for extremely large sizes by adding a final fallback
return after the loop (e.g., format remaining size with "PB" or clamp to the
largest unit) so the function never returns None, and preferably replace the
manual loop with Django's filesizeformat (from django.template.defaultfilters
import filesizeformat) to ensure consistent formatting with templates.
| @asynccontextmanager | ||
| async def track_session(self, user: User): | ||
| """Context manager to track API session for the duration of a request.""" | ||
| session = await self._create_statistic(user) | ||
| try: | ||
| yield session | ||
| finally: | ||
| pass |
There was a problem hiding this comment.
Empty finally block - statistics not recorded on exceptions.
The track_session context manager creates a statistic record but the finally block does nothing. If an exception occurs before _finalize_statistic is called, the request won't be counted. Consider whether failed requests should also be tracked.
If you want to track all requests (including failed ones):
@asynccontextmanager
async def track_session(self, user: User):
"""Context manager to track API session for the duration of a request."""
session = await self._create_statistic(user)
+ finalized = False
try:
yield session
finally:
- pass
+ if not finalized:
+ await self._finalize_statistic(session, transfer_size=0)Then set finalized = True before calling _finalize_statistic in each view, or restructure to pass transfer_size back to the context manager.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In adit/dicom_web/views.py around lines 82 to 89, the asynccontextmanager
track_session creates a statistic but leaves the finally block empty so
statistics aren’t finalized if an exception occurs; fix by ensuring
_finalize_statistic is always called in the finally block (and pass any needed
transfer_size or finalized flag back from the view) or by yielding a mutable
container (e.g., dict) from the context so the view can set transfer_size and
finalized=True, then in finally call await self._finalize_statistic(session,
transfer_size=container.get("transfer_size"),
finalized=container.get("finalized", False)); adjust callers to set
container["transfer_size"] and container["finalized"]=True when appropriate so
requests (including failures if desired) are recorded.
adit/dicom_web/views.py
Outdated
| response_data = [results] | ||
| await self._finalize_statistic(session, transfer_size=sys.getsizeof(response_data)) |
There was a problem hiding this comment.
Inconsistent transfer size calculation - sys.getsizeof vs serialized size.
sys.getsizeof(response_data) returns the Python object's memory footprint, not the actual response size sent to the client. Other views use len(json.dumps(response_data).encode()) for consistency.
- response_data = [results]
- await self._finalize_statistic(session, transfer_size=sys.getsizeof(response_data))
+ response_data = [results.to_json_dict()]
+ await self._finalize_statistic(
+ session, transfer_size=len(json.dumps(response_data).encode())
+ )Note: You'll need to verify how results (a Dataset) should be serialized. If the renderer handles this differently, you may need to adjust the serialization approach accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response_data = [results] | |
| await self._finalize_statistic(session, transfer_size=sys.getsizeof(response_data)) | |
| response_data = [results] | |
| await self._finalize_statistic(session, transfer_size=len(json.dumps(response_data, default=str).encode())) |
Code Review - PR #271: API StatisticsThank you for implementing API usage tracking! This is a useful feature for monitoring user activity. I've reviewed the changes and have the following feedback: Code Quality & ArchitectureStrengths:
Issues & Recommendations:
Performance ConsiderationsIssues:
Security ConcernsMedium Priority:
Test CoverageCritical Issue: No tests added for the new functionality - This is a significant concern. The PR adds substantial new functionality but includes no tests. You should add:
MigrationThe migration file looks good:
Admin InterfaceMinor Issue: In def get_size(self, obj):
size = obj.total_transfer_size
for unit in ["B", "KB", "MB", "GB", "TB", "PB"]:
if size < 1024:
return f"{size:.2f} {unit}"
size /= 1024
return f"{size:.2f} PB" # Add this lineSummaryBefore merging, please address:
The architecture and approach are solid, but the lack of tests and performance concerns with JSON serialization need to be addressed before merging. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
adit/dicom_web/views.py (1)
81-88: Emptyfinallyblock leaves requests untracked on exceptions.The context manager's
finallyblock does nothing. If an exception occurs before_finalize_statisticis called (e.g., duringqido_findorwado_retrieve), the request won't be counted in statistics. For streaming responses,_StreamingSessionWrapperhandles finalization separately, but non-streaming paths could lose tracking on errors.Consider whether failed requests should also increment
total_number_requests:@asynccontextmanager async def track_session(self, user: User): """Context manager to track API session for the duration of a request.""" session = await self._create_statistic(user) + finalized = False try: yield session finally: - pass + if not finalized: + await self._finalize_statistic(session, transfer_size=0)Then set
finalized = Truebefore the explicit_finalize_statisticcalls in each view. Alternatively, if only successful requests should be tracked, document this behavior.
🧹 Nitpick comments (1)
adit/dicom_web/views.py (1)
317-317: Replacegetattrwith direct attribute access.Using
getattrwith a constant attribute name provides no benefit over direct attribute access.- renderer = cast(DicomWebWadoRenderer, getattr(request, "accepted_renderer")) + renderer = cast(DicomWebWadoRenderer, request.accepted_renderer)This same pattern appears at lines 354, 382, 420, 454, and 498.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
adit/core/templates/core/admin_section.html(1 hunks)adit/dicom_web/migrations/0004_apiusage.py(1 hunks)adit/dicom_web/models.py(2 hunks)adit/dicom_web/views.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
adit/dicom_web/models.py (1)
adit/core/models.py (7)
Meta(28-29)Meta(84-85)Meta(113-120)Meta(204-212)Meta(364-371)Meta(404-406)Meta(480-481)
adit/dicom_web/views.py (3)
adit/core/utils/dicom_dataset.py (6)
QueryDataset(140-252)dataset(126-127)dataset(156-157)get(129-130)SOPInstanceUID(114-115)StudyInstanceUID(53-54)adit/dicom_web/models.py (1)
APIUsage(18-35)adit/dicom_web/utils/wadors_utils.py (1)
wado_retrieve(19-97)
🪛 Ruff (0.14.7)
adit/dicom_web/models.py
29-31: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
32-32: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
adit/dicom_web/views.py
93-93: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
317-317: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
354-354: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
382-382: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
420-420: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
454-454: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
498-498: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
adit/dicom_web/migrations/0004_apiusage.py
11-14: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
16-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (7)
adit/dicom_web/migrations/0004_apiusage.py (1)
1-58: LGTM!The migration is well-structured with appropriate field types, proper index on
time_last_accessedfor efficient ordering, and a unique constraint onownerto ensure one usage record per user. The static analysis warnings aboutClassVarare false positives for auto-generated Django migrations.adit/core/templates/core/admin_section.html (1)
31-51: LGTM!The API Usage section follows the existing table pattern, uses appropriate Django template filters like
filesizeformatfor human-readable file sizes, and integrates cleanly with the existing admin section layout.adit/dicom_web/models.py (1)
18-35: LGTM!The model design is appropriate for aggregate per-user API statistics. Using
BigIntegerFieldfor transfer size handles large cumulative values well. The unique constraint onownereffectively creates a one-to-one relationship, though semantically aOneToOneFieldcould also work here. The current approach is valid and aligns with patterns inadit/core/models.py.adit/dicom_web/views.py (4)
67-79: Well-designed atomic update pattern.Using
F()expressions for atomic updates prevents race conditions when multiple concurrent requests update the same user's statistics. This is the correct approach for counter-like fields.
276-291: Good design for streaming transfer size tracking.The
_StreamingSessionWrappercorrectly accumulates chunk sizes during streaming and ensures finalization happens in thefinallyblock, guaranteeing statistics are recorded even if streaming is interrupted.
549-552: Consistent transfer size calculation for STOW-RS responses.The calculation correctly serializes the Dataset to JSON for size measurement, matching the pattern used in other views.
318-323: Session object remains valid for finalization.The
sessionobject is captured by_StreamingSessionWrapperbefore the context manager exits, and_finalize_statisticsafely uses only itspkattribute viaAPIUsage.objects.filter(pk=statistic.pk).update(...). Since the primary key is immutable and the ORM update executes at the database level (not dependent on session attachment), the session remains usable for finalization even after theasync with self.track_session(...)context exits. The__aiter__finally block correctly ensures_finalize_statisticis called to complete the update.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.