-
Notifications
You must be signed in to change notification settings - Fork 109
fix: Add JSON formatter for uvicorn access logs #1733
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
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
|
Related PR for V0 legacy server (OpenHands main repo): OpenHands/OpenHands#12442 |
eed63aa to
429c700
Compare
The current uvicorn access logs pass HTTP data in record.args as a tuple,
but the standard JsonFormatter doesn't extract these into structured fields.
This results in the HTTP information only being available in the message string.
This commit adds UvicornAccessJsonFormatter that properly extracts HTTP
fields from record.args into structured JSON attributes:
- http.client_ip
- http.method
- http.url
- http.version
- http.status_code
Before:
{"message": "10.2.66.141:47584 - \"POST /api/conversations HTTP/1.1\" 201"}
After:
{"message": "...", "http.client_ip": "10.2.66.141:47584",
"http.method": "POST", "http.url": "/api/conversations",
"http.version": "1.1", "http.status_code": 201}
Co-authored-by: openhands <openhands@all-hands.dev>
429c700 to
64a4cb9
Compare
all-hands-bot
left a comment
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.
Overall the PR looks good and solves the stated problem effectively. I have a couple of minor suggestions for code quality improvements.
| log_data["http.method"] = method | ||
| log_data["http.url"] = full_path | ||
| log_data["http.version"] = http_version | ||
| log_data["http.status_code"] = int(status_code) # type: ignore[call-overload] |
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.
🟡 Suggestion: The type: ignore comment suppresses a type checking warning for the int conversion. Since status_code comes from uvicorn's log args tuple, it should already be an int. Consider either:
- Removing the conversion if it's guaranteed to be an int
- Adding explicit validation to handle edge cases
- Adding a try-except to handle potential conversion errors gracefully
This would make the code more defensive and easier to maintain if uvicorn's logging format changes.
| else: | ||
| # Non-JSON mode: propagate all to root (uses Rich handler) | ||
| config["loggers"] = { | ||
| "uvicorn": { |
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.
🟡 Suggestion: The uvicorn and uvicorn.error logger configurations here (lines 100-109) are identical to lines 80-89. Consider extracting the common configuration outside the if/else block to reduce duplication and make the code more maintainable.
|
@OpenHands the above comments are good, reflect them |
|
I'm on it! neubig can track my progress at all-hands.dev |
- Replace type: ignore with explicit type checking for status_code to handle edge cases gracefully (int, str, or fallback to original) - Extract common uvicorn/uvicorn.error logger configs outside if/else block to reduce code duplication - Store log_level in variable to avoid repeated getLevelName calls Co-authored-by: openhands <openhands@all-hands.dev>
|
I've addressed the review comments on PR #1733 and pushed the changes. Here's a summary of what was done: Changes Made1. Fixed status_code type handling (addressing comment on line 39)
2. Reduced code duplication (addressing comment on line 100)
Verification
The changes have been pushed to the |
Summary
This PR fixes incomplete JSON logging for uvicorn access logs in the agent-server.
Problem
Currently, when
LOG_JSON=trueor in CI environments, uvicorn access logs appear in Datadog with a raw message string like:While the logs are technically JSON (they have
asctime,levelname, etc.), the HTTP fields are not extracted into separate attributes, making it difficult to:Solution
Added a custom
AccessLogJsonFormatterthat extracts HTTP fields from uvicorn's access log records into structured JSON attributes:{ "asctime": "2026-01-15 15:24:08,131", "levelname": "INFO", "name": "uvicorn.access", "message": "10.2.66.141:47584 - \"POST /api/conversations HTTP/1.1\" 201", "http.client_ip": "10.2.66.141:47584", "http.method": "POST", "http.url": "/api/conversations", "http.version": "1.1", "http.status_code": 201 }Changes
AccessLogJsonFormatterclass that extendsJsonFormatterto extract HTTP fields fromrecord.argsget_uvicorn_logging_config()to use the custom formatter whenLOG_JSON=trueorCI=trueTesting
Verified locally that the formatter correctly extracts HTTP fields:
Output:
{"asctime": "2026-01-15 15:24:08,131", "levelname": "INFO", "name": "uvicorn.access", "message": "10.2.66.141:47584 - \"POST /api/conversations HTTP/1.1\" 201", "http.client_ip": "10.2.66.141:47584", "http.method": "POST", "http.url": "/api/conversations", "http.version": "1.1", "http.status_code": 201}@neubig can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:58aa049-pythonRun
All tags pushed for this build
About Multi-Architecture Support
58aa049-python) is a multi-arch manifest supporting both amd64 and arm6458aa049-python-amd64) are also available if needed