Logging fixes and enhancements #421
Conversation
rsbowman-striveworks
requested review from
czaloom,
ntlind and
ekorman
as code owners
czaloom
force-pushed
the
rsb-logging-fixes
branch
from
56f9a30 to
77e1be6
Compare
api/velour_api/logging.py
Outdated
| def log_request( | ||
| request: Request, ignore_paths=frozenset(["/health", "/ready"]) | ||
| ): | ||
| if request.url.path in ignore_paths: | ||
| return |
There was a problem hiding this comment.
i'd think we want to log requests if health or ready aren't 200?
There was a problem hiding this comment.
Good point. Currently we only log the request, but the uvicorn logging included status code. We probably want that also. I'll work on it.
This gets rid of the "INFO" level endpoint logging which was made redundant by the addition of structlog logging
rsbowman-striveworks
force-pushed
the
rsb-logging-fixes
branch
from
77e1be6 to
ab2a1fd
Compare
|
Ideally we could replace uvicorn logging with something similar but structured. I haven't been able to do that. In particular, this PR does no logging for requests to endpoints that don't exist, whereas uvicorn shows something like
I think the best way forward would be to enhance the uvicorn logging instead of replacing it (like this uvicorn PR), but that doesn't seem possible right now. I'm not very happy with this PR but it's better than what we have currently. |
Structlog logging wasn't working because the
BackgroundTaskin the custom route handler was never run. Fix by attaching the task to the response. FastAPI doesn't make this easy, see here for discussion.Uvicorn still logs every request. We don't need this now with the custom endpoint logger, so turn up the log level with
--log-level warning. I have never seen a uvicorn warning, and maybe they're important, so I kept those in. I'm open to other ways to do this, the command line flag worked and was convenient so I did that.