Repository navigation
198: Track client-closed requests as 499 in usage logging - #199
JaeYeonLee0621 wants to merge 4 commits into
Conversation
| request_log.processing_time_ms = int((response_start_time - kwargs["request_start"]) * 1000) | ||
| request_log.response_time_ms = int((end_time - response_start_time) * 1000) | ||
| request_log.status_code = result.status_code | ||
|
|
There was a problem hiding this comment.
- Not streaming (JsonResponse) :
log_requestsavesresult.status_codeas-is - Streaming :
log_requestleaves it None, and_openai_streamfigures out the right code when the stream actually ends
There was a problem hiding this comment.
Maybe I am misunderstanding but should we not set status_code to 499 here if it is None?
There was a problem hiding this comment.
Or can non-streaming requests never lead to a disconnect this way? What happens if a client disconnects in a non streaming request before the result is returned? I guess we still wait for a response and then just take that status code? But I guess the whole request in Django is then closed so do we even listen to the response then?
There was a problem hiding this comment.
I may be wrong, but I'm not sure if client disconnects are handled at all for non-streaming responses. The docs only mention it for StreamingHttpResponse. If I were to guess, I'd say you'd probably get a broken pipe error or something when Django finishes processing and tries to write the response - I imagine this would show up in the logs though? But that's just a guess on my side.
There was a problem hiding this comment.
I don't know about this one that much, but I studied with deepseek 🐳 for a while, as far as I understand :
- In Django's
ASGIHandler, when a client disconnects while the view is still awaiting the upstream. - It cancels the request task, which raises
asyncio.CancelledErrorinside the view. (not a broken pipe, because the task is cancelled before any write attempt) - The except
asyncio.CancelledErrorcatches it and records 499 for non-streaming responses too.
|
|
||
| request_log.status_code = 200 | ||
| yield "data: [DONE]\n\n" | ||
| except (asyncio.CancelledError, GeneratorExit): |
There was a problem hiding this comment.
GeneratorExit: Signals a paused generator to immediately stop producing data and clean up because the consumer called .close().asyncio.CancelledError: Signals a paused coroutine or async task to abort immediately because the task was cancelled.Why Exception cannot catch: Both inherit directly fromBaseExceptionrather thanException.
| return Usage(input_tokens=0, output_tokens=0) | ||
|
|
||
|
|
||
| def _openai_stream( |
There was a problem hiding this comment.
catch_router_exceptions: runs at the very start, only ever report upstream errors_openai_stream: runs during streaming, by the time_openai_streamis running, both things can happen there: the client closing, and the upstream failing mid-stream
meffmadd
left a comment
There was a problem hiding this comment.
Looks very good! I have one quick question for how non-streaming requests should be handled.
| request_log.processing_time_ms = int((response_start_time - kwargs["request_start"]) * 1000) | ||
| request_log.response_time_ms = int((end_time - response_start_time) * 1000) | ||
| request_log.status_code = result.status_code | ||
|
|
There was a problem hiding this comment.
Maybe I am misunderstanding but should we not set status_code to 499 here if it is None?
| request_log.processing_time_ms = int((response_start_time - kwargs["request_start"]) * 1000) | ||
| request_log.response_time_ms = int((end_time - response_start_time) * 1000) | ||
| request_log.status_code = result.status_code | ||
|
|
There was a problem hiding this comment.
Or can non-streaming requests never lead to a disconnect this way? What happens if a client disconnects in a non streaming request before the result is returned? I guess we still wait for a response and then just take that status code? But I guess the whole request in Django is then closed so do we even listen to the response then?
|
|
||
| response_start_time = time.monotonic() | ||
| result: HttpResponse | StreamingHttpResponse = await view_func(request, *args, **kwargs) | ||
|
|
There was a problem hiding this comment.
I add try, except for non-streaming requests to save 499 status code and stop running Django logic.
natkam
left a comment
There was a problem hiding this comment.
I'd just remove that if __name__ = ... in the test file, but generally it looks good to me!
| request_log.processing_time_ms = int((response_start_time - kwargs["request_start"]) * 1000) | ||
| request_log.response_time_ms = int((end_time - response_start_time) * 1000) | ||
| request_log.status_code = result.status_code | ||
|
|
There was a problem hiding this comment.
I may be wrong, but I'm not sure if client disconnects are handled at all for non-streaming responses. The docs only mention it for StreamingHttpResponse. If I were to guess, I'd say you'd probably get a broken pipe error or something when Django finishes processing and tries to write the response - I imagine this would show up in the logs though? But that's just a guess on my side.
status_codeis left asNULL, so these requests don't count as failures on the dashboard.499. The dashboard countsstatus codes ≥ 400as failures, so these requests will show up there.Notes
ex)