-
Notifications
You must be signed in to change notification settings - Fork 214
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
Add structured logging for the API #4263
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.
The logs now look awesome, and you can follow a request through all of the log statements.
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.
Awesome! Two small blockers:
- The issue requests
For ease of reading in local development, add a switch to default to flat line style logs when ENVIRONMENT == "local".
, and indeed, the JSON logs are very hard to read with my human eyes. I've left a suggestion for implementing this, but would be happy with anything that makes it configurable, with sensible defaults per environment. - We have a documentation website page about logging, that doesn't necessarily need to be completely re-written now (some parts of the app still use that approach, until they are covered by the other structured logging issues), but at least a reference to structlog in the API for now would be good to put there in case it is referenced.
These are small blockers, but I feel they're blockers because the first is an important devex problem that the issue anticipated and requested; the second is a documentation issue that if left unaddressed would contradict the state of the code after this PR is merged.
except ConnectionError: | ||
cache_fetch_failed = True | ||
sources = None | ||
log.warning("Redis connect failed, cannot get cached sources.") | ||
logger.warning("Redis connect failed, cannot get cached sources.") |
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 wonder if it would be nice to change these to structured logs too, maybe in a future issue. Something like logger.warning("Redis connect failed", when="get-cached-sources")
, and so forth. Ideally these logs never matter though, and would never be meaningful to query, so probably not worth it!
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.
Yes, we should definitely update existing logs to be more "structured" but that, like you said, will have to be a separate issue and PR.
Full-stack documentation: https://docs.openverse.org/_preview/4263 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. Changed files 🔄: |
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.
Excellent!
Fixes
Fixes #3432 by @sarayourfriend
Description
This PR
django-log-request-id
as that functionality is included indjango-structlog
structlog
Please don't be daunted by the file count, most of these are
logging
→structlog
migrations. These changes was required to make the tests pass, else I would've made them a separate PR. The main files to review are the 5 or so changed files insideapi/conf/
.The PR is marked as 🟧 high-priority because it blocks all the work on monitoring metrics associated with content moderation.
Testing Instructions
just api/up
.just logs web
.request_started
event logging the URL, HTTP method and user agent.request_finished
event logging the URL, HTTP method and response status code.getChild
logger as each log entry records the logger name, file name, func name and even line where it was called.{"event": "<message>"}
.Checklist
Update index.md
).main
) or a parent feature branch.just catalog/generate-docs
for catalogPRs) or the media properties generator (
just catalog/generate-docs media-props
for the catalog or
just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin