-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/44 show content size #46
Conversation
@danielmursa-dev please rebase your changes into a single commit, after that @stevenbal can do a review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
- Coverage 97.75% 96.36% -1.39%
==========================================
Files 21 22 +1
Lines 445 468 +23
Branches 57 29 -28
==========================================
+ Hits 435 451 +16
- Misses 6 13 +7
Partials 4 4 ☔ View full report in Codecov by Sentry. |
@stevenbal please can you do a review, thanks |
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.
Looks good overall, some minor comments. Make sure to also add Fixes: #<issue number>
to the description of the pull request, this links the PR to the issue and closes it once the PR is merged.
It looks like the commits aren't squashed though, it would be nice if there are two commits (one for the implementation and another to add tests). You can do this with interactive rebase git rebase -i <first commit hash>^
and choosing squash
for the commits that should be squashed or by resetting the commits with git reset HEAD~<number of commits to reset>
and recreating the manually
@@ -76,6 +78,7 @@ class OutgoingRequestsLogAdmin(admin.ModelAdmin): | |||
"response_ms", | |||
"res_headers", | |||
"res_content_type", | |||
"response_content_length", |
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 think it would be nice if it's possible to sort by this field in the admin listview, that way it's easier to find requests with unexpectedly large response bodies. Since it's a computed field, you might have to annotate the queryset to implement this: https://docs.djangoproject.com/en/4.2/ref/models/querysets/#annotate
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'm absolutely agree with making it sortable, but I think it is not possible to annotate the queryset because this property reads the length of another computed property response_body_decoded
. I think it is therefore for this reason, I could be wrong.
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.
Hmm that's true, though I do wonder how useful this if we cannot sort by length.
I think it's only possible to make it sortable if we use res_body
instead of response_body_decoded
, although I do think the lengths won't be exactly correct depending on the content_type of the response.
Example for an HTML response
>>> len(large.res_body)
233210
>>> len(large.response_body_decoded)
233181
We could do something like this in that case:
from django.db.models import Func
class LengthFunction(Func):
function = 'LENGTH'
def get_queryset(self, request):
qs = super().get_queryset(request)
return qs.annotate(
annotated_response_length=LengthFunction('response_body_decoded')
)
def response_content_length(self, obj):
return obj.annotated_response_length
response_content_length.admin_order_field = 'annotated_response_length'
@alextreme what are your thought on this. Would it be okay if the response_content_length
in the list view isn't exactly correct, but it would be sortable? Or would you rather have the exact length without sortability?
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.
@stevenbal sorry guys I overlooked this one.
I think going for an exact-length (or inexact, a ballpark is fine) explicitly without sortability would be preferable, mostly because I see performance problems occur with such an annotation over millions of response-bodies... I'd say do what's quickest when storing the responses and disallow sorting in the admin on this column
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.
hmm yeah performance might be an issue indeed.
@danielmursa-dev let's leave this as it is now
cd3d202
to
23b483a
Compare
Fixes: #44