-
Notifications
You must be signed in to change notification settings - Fork 45.8k
feat(backend): setup logging utilities #10027
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: dev
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
✅ Deploy Preview for auto-gpt-docs canceled.
|
Here's the code health analysis summary for commits Analysis Summary
|
@@ -30,6 +30,7 @@ def lint(): | |||
["ruff", "format", "--diff", "--check", LIBS_DIR], | |||
["isort", "--diff", "--check", "--profile", "black", BACKEND_DIR], | |||
["black", "--diff", "--check", BACKEND_DIR], | |||
["python", "check_logging.py"], |
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.
Add to format step too
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
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.
What specific issue(s) does this PR aim to solve?
Code needs some work.
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.
put this in a scripts
folder
if PATTERN.search(text) and "TruncatedLogger" not in text: | ||
errors.append(str(path.relative_to(ROOT))) | ||
|
||
if errors: | ||
print("Plain logging.getLogger usage detected in:\n" + "\n".join(errors)) |
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.
So any usage of logger.getLogger
not wrapped in TruncatedLogger
is now reported as a linting error?
But this PR doesn't fix all usages of logging.getLogger
so the PR wouldn't pass this check.
If we needed system-wide logging truncation, add some middleware in configure_logging
to do this rather than wrapping all uses across the codebase in a helper class.
But the last time we discussed this, there was no real need for this, because there are only a few specific places where messages with long "attachments" are being logged.
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 script always exits normally, so it will never trigger CI or pre-commit check failure
- repo: local | ||
hooks: | ||
- id: check-loggers | ||
name: Check for plain loggers |
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.
name: Check for plain loggers | |
name: Check for plain loggers - AutoGPT Platform - Backend |
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.
why is this needed?
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.
why is this needed?
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.
why is this needed?
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.
why is this needed?
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.
why is this needed?
YYYY-MM-DD HH:MM:SS LEVEL [PREFIX] message {"json_fields": {...}} | ||
``` | ||
|
||
When creating loggers for user-facing modules, wrap them with |
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.
what is a user-facing module?
Summary
Testing
poetry -C autogpt_platform/backend run format
poetry -C autogpt_platform/backend run lint
poetry -C autogpt_platform/backend run test
(fails: Error 111 connecting to localhost:6379)