Universal HTTP logging #1254
Universal HTTP logging #1254
Conversation
gastonmorixe
force-pushed
the
gm/feature/logging
branch
3 times, most recently
from
85db9b6 to
a0e6126
Compare
|
|
@codex review please and give your thoughts / suggestions |
|
@codex can you review please? |
|
@codex review deeply please |
- CLI: add --http-logging and --http-debug flags (env: LLM_HTTP_LOGGING, LLM_HTTP_DEBUG) and early initialization via configure_http_logging(). - utils: implement universal HTTP logging (color formatter, config detection, logging setup); include 'format' in _get_http_logging_config for tests; support NO_COLOR. - Tests: add unit and integration tests for HTTP logging configuration and CLI help. - Ignore build/ and dist/ in .gitignore; remove stray build/ artifact. - Add docs page docs/http-debugging.md with Quick Start, env vars, CLI flags and guidance; link from docs/logging.md and include in main TOC. - Regenerate docs/help.md and README to include new flags and navigation.
The user-facing documentation for the new universal HTTP logging feature was split across `UNIVERSAL-HTTP-LOGGING.md` and `docs/http-debugging.md`. This commit consolidates all the relevant user-facing documentation into `docs/http-debugging.md` and removes the now redundant `UNIVERSAL-HTTP-LOGGING.md` file, which contained mostly implementation details.
The loop that attaches handlers only runs when logger.handlers is empty. The OpenAI SDK (and some others) ship their loggers with a logging.NullHandler pre‑attached, so this condition is false and your code never replaces the NullHandler. As a result, even after --http-logging the OpenAI logger still drops all messages and nothing is emitted. To make the feature actually show provider logs, the setup should detect and replace existing NullHandlers (or always add your stream handler) instead of skipping loggers that already have a handler. Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
gastonmorixe
force-pushed
the
gm/feature/logging
branch
from
6ce9632 to
53840ef
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds universal HTTP logging functionality to LLM, extending debugging capabilities beyond OpenAI to all providers (Anthropic, Gemini, etc.). The implementation includes CLI flags, environment variable configuration, colored log formatting, and comprehensive documentation.
Key changes:
- Implemented universal HTTP logging system with INFO and DEBUG levels
- Added
--http-logging,--http-debug, and--no-colorCLI flags - Created custom
HTTPColorFormatterfor color-coded, readable HTTP logs
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
llm/utils.py |
Adds HTTPColorFormatter class and HTTP logging configuration functions (configure_http_logging, _get_http_logging_config, is_http_logging_enabled) |
llm/cli.py |
Integrates HTTP logging flags into main CLI command, sets environment variables, and calls configure_http_logging() on initialization |
tests/test_http_logging.py |
Unit tests for HTTP logging configuration, CLI integration, and documentation validation |
tests/test_http_logging_integration.py |
Integration tests for HTTP logging with real CLI commands and mocked HTTP requests |
docs/http-debugging.md |
Comprehensive documentation covering HTTP logging setup, usage examples, troubleshooting, and security considerations |
docs/logging.md |
Adds cross-reference to HTTP debugging documentation |
docs/index.md |
Adds http-debugging to documentation table of contents |
docs/help.md |
Updates CLI help output to include new HTTP logging flags and environment variables |
README.md |
Adds HTTP debugging section to README with links to comprehensive documentation |
.gitignore |
Adds build/ and dist/ directories to gitignore |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Replace NullHandlers with our custom handler | ||
| if all(isinstance(h, logging.NullHandler) for h in logger.handlers): | ||
| logger.handlers.clear() | ||
| handler = logging.StreamHandler() | ||
| handler.setFormatter(formatter) | ||
| logger.addHandler(handler) | ||
| logger.propagate = False | ||
| logger.addHandler(handler) | ||
| logger.propagate = False |
There was a problem hiding this comment.
The variable 'logger' is referenced but never defined. This line appears to be checking 'logger.handlers' but 'logger' doesn't exist in this scope. This code block should iterate over the 'http_loggers' list defined on line 383, or it should define which logger is being checked.
| # Replace NullHandlers with our custom handler | |
| if all(isinstance(h, logging.NullHandler) for h in logger.handlers): | |
| logger.handlers.clear() | |
| handler = logging.StreamHandler() | |
| handler.setFormatter(formatter) | |
| logger.addHandler(handler) | |
| logger.propagate = False | |
| logger.addHandler(handler) | |
| logger.propagate = False | |
| # Replace NullHandlers with our custom handler for each HTTP logger | |
| for logger_name in http_loggers: | |
| logger = logging.getLogger(logger_name) | |
| # Only replace if all existing handlers are NullHandlers | |
| if logger.handlers and all( | |
| isinstance(h, logging.NullHandler) for h in logger.handlers | |
| ): | |
| logger.handlers.clear() | |
| handler = logging.StreamHandler() | |
| handler.setFormatter(formatter) | |
| logger.addHandler(handler) | |
| logger.propagate = False |
| # Replace NullHandlers with our custom handler | ||
| if all(isinstance(h, logging.NullHandler) for h in logger.handlers): | ||
| logger.handlers.clear() | ||
| handler = logging.StreamHandler() | ||
| handler.setFormatter(formatter) | ||
| logger.addHandler(handler) | ||
| logger.propagate = False | ||
| logger.addHandler(handler) | ||
| logger.propagate = False |
There was a problem hiding this comment.
The 'http_loggers' list is created but never used. The code defines a list of logger names but doesn't iterate over them to configure handlers. This appears to be incomplete implementation - the code should likely iterate through 'http_loggers' to configure each logger with the custom formatter.
| # Replace NullHandlers with our custom handler | |
| if all(isinstance(h, logging.NullHandler) for h in logger.handlers): | |
| logger.handlers.clear() | |
| handler = logging.StreamHandler() | |
| handler.setFormatter(formatter) | |
| logger.addHandler(handler) | |
| logger.propagate = False | |
| logger.addHandler(handler) | |
| logger.propagate = False | |
| # Replace NullHandlers with our custom handler and ensure formatter is applied | |
| for name in http_loggers: | |
| logger = logging.getLogger(name) | |
| if not logger.handlers or all( | |
| isinstance(h, logging.NullHandler) for h in logger.handlers | |
| ): | |
| logger.handlers.clear() | |
| handler = logging.StreamHandler() | |
| handler.setFormatter(formatter) | |
| logger.addHandler(handler) | |
| # Prevent duplicate logging via root logger | |
| logger.propagate = False |
| # Replace NullHandlers with our custom handler | ||
| if all(isinstance(h, logging.NullHandler) for h in logger.handlers): | ||
| logger.handlers.clear() | ||
| handler = logging.StreamHandler() | ||
| handler.setFormatter(formatter) | ||
| logger.addHandler(handler) | ||
| logger.propagate = False | ||
| logger.addHandler(handler) | ||
| logger.propagate = False |
There was a problem hiding this comment.
The handler is being added twice (lines 392 and 394), and 'logger.propagate' is being set twice (lines 393 and 395). This appears to be duplicated code. Additionally, since 'logger' is undefined (see line 388), this entire block will fail at runtime.
| # Replace NullHandlers with our custom handler | |
| if all(isinstance(h, logging.NullHandler) for h in logger.handlers): | |
| logger.handlers.clear() | |
| handler = logging.StreamHandler() | |
| handler.setFormatter(formatter) | |
| logger.addHandler(handler) | |
| logger.propagate = False | |
| logger.addHandler(handler) | |
| logger.propagate = False | |
| # Replace NullHandlers with our custom handler | |
| for logger_name in http_loggers: | |
| logger = logging.getLogger(logger_name) | |
| if all(isinstance(h, logging.NullHandler) for h in logger.handlers): | |
| logger.handlers.clear() | |
| handler = logging.StreamHandler() | |
| handler.setFormatter(formatter) | |
| logger.addHandler(handler) | |
| logger.propagate = False |
| - LLM_HTTP_VERBOSE=1: Enable DEBUG-level HTTP logging | ||
| - LLM_OPENAI_SHOW_RESPONSES=1: Backward compatibility (INFO level) | ||
| """ | ||
| import logging |
|
|
||
| ⚠️ **Important Security Notes:** | ||
|
|
||
| - HTTP logs may contain API keys in headers (these are automatically redacted) |
There was a problem hiding this comment.
The statement that API keys in HTTP headers "are automatically redacted" is overly broad given the current logging implementation. The universal HTTP logging hooks rely on the underlying HTTP libraries’ log output, which can include full URLs (with query parameters like ?key=...) and non-Authorization headers, so API keys and tokens sent via query strings or headers such as X-API-Key/x-goog-api-key will still be emitted in cleartext. To prevent accidental credential leaks when users share or persist logs, this section should be updated to warn that HTTP logs may contain unredacted secrets in both headers and URLs, and not imply comprehensive automatic redaction.
| - HTTP logs may contain API keys in headers (these are automatically redacted) | |
| - HTTP logs may contain API keys and other secrets in headers and URLs (only some fields like `Authorization` are automatically redacted) | |
| - API keys sent via query parameters (for example, `?key=...`) or non-`Authorization` headers (for example, `X-API-Key`, `x-goog-api-key`) may appear in logs in cleartext |
1 participant
Important
Disclaimer: I vibe-coded this logging feature with codex-cli and claude code. I'm not a python expert at all. This should be carefully reviewed.
--http-loggingand--http-debugflags (env:LLM_HTTP_LOGGING,LLM_HTTP_DEBUG) and early initialization via configure_http_logging()._get_http_logging_configfor tests; supportNO_COLOR.