-
Notifications
You must be signed in to change notification settings - Fork 0
Background task db logging #94
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: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This pull request refactors the endpoint monitoring system to use asynchronous background tasks instead of blocking thread pool execution. The change improves API response times by deferring endpoint usage logging until after the response is sent to the client.
Key changes:
- Replaced synchronous
run_in_threadpoolexecution with Starlette'sBackgroundTaskfor non-blocking endpoint registration - Added comprehensive error handling and logging for endpoint registration failures
- Moved response generation to occur before endpoint logging setup
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/app/middleware/monitor_requests.py | Refactored middleware to use BackgroundTask and moved response generation before logging setup |
| src/app/services/sql_service.py | Added try-except block and error logging to register_endpoint method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| session_id=session_id, | ||
| http_code=200, | ||
| response.background = BackgroundTask( | ||
| register_endpoint, request.url.path, session_id, 200 |
Copilot
AI
Jan 5, 2026
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.
The HTTP status code is hardcoded to 200, but response.status_code should be used instead to accurately log the actual response status (e.g., 404, 500). Replace 200 with response.status_code.
| register_endpoint, request.url.path, session_id, 200 | |
| register_endpoint, request.url.path, session_id, response.status_code |
| except Exception as e: | ||
| logger.error(f"Failed to register endpoint {request.url.path}: {e}") |
Copilot
AI
Jan 5, 2026
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 exception handler will never catch exceptions from register_endpoint because BackgroundTask executes after the response is returned. The try-except block should be removed from the middleware since error handling is now in sql_service.py's register_endpoint method.
This pull request improves how API endpoint usage is logged and monitored. The main change is that the logging of endpoint requests is now handled asynchronously in the background, which should improve response times and reliability. Additionally, error handling and logging have been enhanced to ensure failures in logging do not affect the main application flow.
Middleware improvements:
MonitorRequestsMiddlewareinmonitor_requests.pyto useBackgroundTaskfor registering endpoint usage asynchronously, instead of blocking the response with a synchronous call. This allows the API to return responses faster and more reliably. [1] [2]Logging and error handling:
sql_service.pyfor failures during endpoint registration, ensuring that issues are captured without impacting the user experience. [1] [2]