Issue #172 Added response_time to Request model and the necessary adjustments to RequestMiddleware #270
Conversation
- Created migration for new response_time field - Modified RequestMiddlware to not use MiddlewareMixin, record the response time and also create a hook for capturing the Request instance that is created
kylef
left a comment
kylef
left a comment
There was a problem hiding this comment.
Neat!
While I'm not sure we should nessecerily change the design, however one thing that may be important for users of django-requests relying on the information to be aware of is the duration/latency recorded may include client/network latency. Depending on HTTP server, this time may include the client/network time for reading request bodies. I.e, when a client slowly streams a request body slower than the server is using request.read() then the response duration can include request latency and client latency too. A potential solution is to wrap the request's stream (or provide user's a way to deduct time) to reduce read times from overall latency. That would allow making the distinction of service latency vs other latencies. I would expect that most user's of this duration/latency field may expect the time is the server processing time and not latencies induced by the client.
request/middleware.py
Outdated
| response_time (float, optional): The request/response cycle time. Defaults to None. | ||
| Returns: | ||
| request.Request: The request.Request instance that was created |
There was a problem hiding this comment.
This does not appear to be fully true, under validation handling None may be returned.
There was a problem hiding this comment.
How would you recommend changing it?
There was a problem hiding this comment.
Also, I just caught something else. The "ifs" should also be returning None not the response.
1.) switched time.time to time.monotonic 2.) switched response_time to be a DurationField and moved its position in the model 3.) changed doc string on the middleware
|
Any idea what is failing on Tests / lint? |
Please check the |
|
Hey ! This would be very useful here. Is there anything I can do to help get this merged ? (such as rebase/resolve conflicts) As for the duration including network latency, IMO just having the help_text explaining it would be fine, it's already much more valuable than no timing info at all and one wouldn't expect too accurate info from such a simple package anyways. |
| start_time = time.monotonic() | ||
| response = self.get_response(request) | ||
| end_time = time.monotonic() | ||
| response_time = timedelta(end_time - start_time) |
There was a problem hiding this comment.
| response_time = timedelta(end_time - start_time) | |
| response_time = timedelta(seconds=end_time - start_time) |
(otherwise the first argument is days !!)
4 participants
Added response_time (FloatField) to Request model. Modified the from_http_request method to accept the response_time
Created migration for new response_time field
Modified RequestMiddlware to not use depreciated MiddlewareMixin. Middleware now captures response_time and passes it to Request.from_http_method.
Created a hook in the middleware for creating the Request instance so that people that want to extend the middleware (like I did) can capture the created instance and perhaps OneToOneFK to it.