Skip to content

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

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,15 @@ repos:
types: [file, python]
language: system

- repo: local
hooks:
- id: check-loggers
name: Check for plain loggers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: Check for plain loggers
name: Check for plain loggers - AutoGPT Platform - Backend

entry: poetry -C autogpt_platform/backend run python check_logging.py
files: ^autogpt_platform/backend/backend/
language: system
pass_filenames: false

- repo: https://github.com/psf/black
rev: 24.10.0
# Black has sensible defaults, doesn't need package context, and ignores
Expand Down
11 changes: 11 additions & 0 deletions autogpt_platform/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,17 @@ Here are some common scenarios where you might use multiple Docker Compose comma

These scenarios demonstrate how to use Docker Compose commands in combination to manage your AutoGPT Platform effectively.

### Logging
The backend configures logging by calling `configure_logging()` at startup.
Logs use the format:

```
YYYY-MM-DD HH:MM:SS LEVEL [PREFIX] message {"json_fields": {...}}
```

When creating loggers for user-facing modules, wrap them with
Copy link
Member

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?

`TruncatedLogger(logging.getLogger(__name__), "[Component]")` to truncate long
messages and include optional metadata.

### Persisting Data

Expand Down
5 changes: 4 additions & 1 deletion autogpt_platform/backend/backend/app.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
if TYPE_CHECKING:
from backend.util.process import AppProcess

logger = logging.getLogger(__name__)
from backend.util.logging import TruncatedLogger, configure_logging

logger = TruncatedLogger(logging.getLogger(__name__), "[App]")


def run_processes(*processes: "AppProcess", **kwargs):
Expand All @@ -31,6 +33,7 @@ def main(**kwargs):
"""
Run all the processes required for the AutoGPT-server (REST and WebSocket APIs).
"""
configure_logging()

from backend.executor import DatabaseManager, ExecutionManager, Scheduler
from backend.notifications import NotificationManager
Expand Down
2 changes: 2 additions & 0 deletions autogpt_platform/backend/backend/exec.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
from backend.app import run_processes
from backend.executor import ExecutionManager
from backend.util.logging import configure_logging


def main():
"""
Run all the processes required for the AutoGPT-server REST API.
"""
configure_logging()
run_processes(ExecutionManager())


Expand Down
2 changes: 2 additions & 0 deletions autogpt_platform/backend/backend/rest.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@
from backend.executor import DatabaseManager
from backend.notifications.notifications import NotificationManager
from backend.server.rest_api import AgentServer
from backend.util.logging import configure_logging


def main():
"""
Run all the processes required for the AutoGPT-server REST API.
"""
configure_logging()
run_processes(
NotificationManager(),
DatabaseManager(),
Expand Down
2 changes: 2 additions & 0 deletions autogpt_platform/backend/backend/scheduler.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
from backend.app import run_processes
from backend.executor.scheduler import Scheduler
from backend.util.logging import configure_logging


def main():
"""
Run all the processes required for the AutoGPT-server Scheduling System.
"""
configure_logging()
run_processes(Scheduler())


Expand Down
2 changes: 2 additions & 0 deletions autogpt_platform/backend/backend/ws.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
from backend.app import run_processes
from backend.server.ws_api import WebsocketServer
from backend.util.logging import configure_logging


def main():
"""
Run all the processes required for the AutoGPT-server WebSocket API.
"""
configure_logging()
run_processes(WebsocketServer())


Expand Down
17 changes: 17 additions & 0 deletions autogpt_platform/backend/check_logging.py
Copy link
Member

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

Copy link
Member

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import pathlib
import re

PATTERN = re.compile(r"logging\.getLogger\(")
ROOT = pathlib.Path(__file__).resolve().parent / "backend"

errors = []

for path in ROOT.rglob("*.py"):
if path.name == "logging.py":
continue
text = path.read_text()
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))
Comment on lines +13 to +17
Copy link
Member

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.

1 change: 1 addition & 0 deletions autogpt_platform/backend/linter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Copy link
Member

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

["pyright", *TARGET_DIRS],
]
lint_error = None
Expand Down