chore: add structured logging with secret redaction#277
Conversation
📝 WalkthroughWalkthroughThe SDK adds structured logging for client initialization and HTTP request lifecycles, configurable logger and body-logging options, redaction helpers, synchronous/asynchronous coverage, a package ChangesStructured logging
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Stream
participant _request_sync
participant HTTPX
participant Logger
Stream->>_request_sync: configure logger and log_bodies
_request_sync->>Logger: http.request.sent
_request_sync->>HTTPX: send HTTP request
HTTPX-->>_request_sync: response or RequestError
_request_sync->>Logger: http.response.received or http.request.failed
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/test_logging.py (2)
10-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse fixtures instead of helper factories and
MockTransport.Replace these helpers with pytest fixtures that inject the client and transport setup without mock objects.
As per coding guidelines, “Use fixtures to inject objects in tests” and “Do not use mocks or mock objects in tests unless directly requested.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_logging.py` around lines 10 - 31, Replace the make_client and make_async_client helper factories in the logging tests with pytest fixtures that provide the configured Stream and AsyncStream clients directly. Inject the logger configuration and transport setup through fixtures, remove the MockTransport-based mock objects, and update affected tests to consume the fixtures.Source: Coding guidelines
42-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup the logging tests into test classes.
The synchronous and asynchronous logging scenarios are currently ungrouped module-level tests.
As per coding guidelines, “Keep tests well organized and use test classes to group similar tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_logging.py` around lines 42 - 153, Group the module-level logging tests into appropriate test classes, separating synchronous scenarios from the asynchronous scenario. Preserve each test’s existing name suffix, assertions, fixtures, and behavior while nesting them under the new classes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@getstream/__init__.py`:
- Line 15: Make the default getstream logger silent by setting its propagate
flag to false after installing the NullHandler in getstream/__init__.py. In
tests/test_http_client.py lines 418-485, attach the capture handler directly to
the getstream logger for intentional default-package event checks. In
tests/test_logging.py lines 91-97, assert that no getstream records are
captured, not merely that test.* records are absent.
In `@getstream/base.py`:
- Around line 94-100: Update the response-body handling around content_type so
media-type matching is case-insensitive and recognizes all JSON media types,
including structured types such as application/problem+json. Ensure every
recognized JSON response is parsed and passed through redact_json_body before
logging, while preserving the existing text fallback when parsing fails or the
response is not JSON.
In `@getstream/logging_utils.py`:
- Around line 5-6: Update the REDACTED_BODY_KEYS set to include api_key,
ensuring JSON request and response body logging redacts it consistently with the
existing REDACTED_QUERY_PARAMS behavior.
In `@tests/test_logging.py`:
- Around line 100-106: Update test_query_redaction to send a request containing
both a secret query parameter and a non-secret parameter. Assert the captured
http.request.sent record omits the secret value while retaining the non-secret
value, removing the empty-query fallback so the test exercises actual redaction.
---
Nitpick comments:
In `@tests/test_logging.py`:
- Around line 10-31: Replace the make_client and make_async_client helper
factories in the logging tests with pytest fixtures that provide the configured
Stream and AsyncStream clients directly. Inject the logger configuration and
transport setup through fixtures, remove the MockTransport-based mock objects,
and update affected tests to consume the fixtures.
- Around line 42-153: Group the module-level logging tests into appropriate test
classes, separating synchronous scenarios from the asynchronous scenario.
Preserve each test’s existing name suffix, assertions, fixtures, and behavior
while nesting them under the new classes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ec2bdf2-988b-4121-bfe3-18501878828f
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdgetstream/__init__.pygetstream/base.pygetstream/logging_utils.pygetstream/stream.pytests/test_http_client.pytests/test_logging.py
…us query-redaction test
|
Re the two nitpicks (pytest fixtures over helper factories + |
mogita
changed the title
|
Titled |
1 participant
Ticket
https://linear.app/stream/issue/CHA-2957/logging
Summary
Adds structured logging via a
logger=kwarg (stdliblogging.Logger) onStream/AsyncStream:client.initialized,http.request.sent,http.response.received,http.request.failedon both sync and async paths. Mandatory redaction of secret query params and known-secret body keys. Opt-inlog_bodieswith a one-shot WARN. No logger = no output (NullHandler).Tests
Full unit suite green;
tests/test_logging.pycovers sync + async events, redaction, and the no-logger case.Summary by CodeRabbit
New Features
Documentation