Isolate telemetry to garth's session using hooks#182
Conversation
|
Warning Rate limit exceeded@matin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughReplaces global logfire instrumentation with session-level telemetry hooks and a pluggable callback API; Telemetry now attaches a response hook to garth's requests.Session, routes sanitized telemetry data via a default logfire sender or a user-provided callback, and adds docs and tests for the new flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Garth Client
participant Telemetry
participant Session as requests.Session
participant Hook as Response Hook
participant Callback as Callback (default or custom)
participant Logfire
Client->>Telemetry: configure(telemetry_enabled, callback?)
Telemetry->>Telemetry: store callback or set default
alt no custom callback
Telemetry->>Telemetry: _configure_logfire() once (if needed)
end
Client->>Telemetry: attach(session)
Telemetry->>Session: append _response_hook to hooks['response']
Note over Session: Later — an HTTP request/response occurs
Session->>Hook: invoke response hook
Hook->>Telemetry: _response_hook(response)
Telemetry->>Telemetry: build & sanitize payload
alt custom callback provided
Telemetry->>Callback: callback(sanitized_payload)
else default callback
Telemetry->>Callback: _default_callback(sanitized_payload)
Callback->>Logfire: POST payload to Logfire ingest
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 68 68
Lines 3381 3486 +105
==========================================
+ Hits 3381 3486 +105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/garth/telemetry.py (1)
97-100: Consider usingWeakSetto avoid potential memory leaks.
_attached_sessionsstoresid(session)values, which are integers. If a session is garbage collected and a new session is created with the sameid(), the checksession_id in self._attached_sessionscould incorrectly returnTrue, skipping hook attachment.Consider using
weakref.WeakSetto store weak references to sessions, which automatically removes entries when sessions are garbage collected.♻️ Suggested implementation using WeakSet
import json import os import re +import weakref from collections.abc import Callable from dataclasses import dataclass, field from requests import Response, Session_configured: bool = field(default=False, repr=False) _logfire_configured: bool = field(default=False, repr=False) - _attached_sessions: set = field(default_factory=set, repr=False) + _attached_sessions: weakref.WeakSet = field( + default_factory=weakref.WeakSet, repr=False + )Then update
attach:def attach(self, session: Session): - session_id = id(session) - if session_id in self._attached_sessions: + if session in self._attached_sessions: return session.hooks["response"].append(self._response_hook) - self._attached_sessions.add(session_id) + self._attached_sessions.add(session)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/configuration.mdsrc/garth/http.pysrc/garth/telemetry.pytests/test_telemetry.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest with VCR cassettes for HTTP recording/playback in tests
Mirror test directory structure to match source code structure
Files:
tests/test_telemetry.py
tests/**
⚙️ CodeRabbit configuration file
tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
Files:
tests/test_telemetry.py
🧠 Learnings (1)
📚 Learning: 2026-01-09T22:41:07.977Z
Learnt from: CR
Repo: matin/garth PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T22:41:07.977Z
Learning: Applies to tests/**/*.py : Use pytest with VCR cassettes for HTTP recording/playback in tests
Applied to files:
tests/test_telemetry.py
🧬 Code graph analysis (2)
src/garth/http.py (1)
src/garth/telemetry.py (1)
attach(212-223)
src/garth/telemetry.py (2)
src/garth/http.py (2)
request(149-188)configure(50-112)tests/conftest.py (1)
session(15-16)
🪛 LanguageTool
docs/configuration.md
[grammar] ~131-~131: Ensure spelling is correct
Context: ...allback instead. ### Use with existing logfire setup If your application already conf...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~131-~131: Ensure spelling is correct
Context: ...ad. ### Use with existing logfire setup If your application already configures l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.14.10)
src/garth/telemetry.py
108-108: Unused method argument: args
(ARG002)
108-108: Unused method argument: kwargs
(ARG002)
137-138: try-except-pass detected, consider logging the exception
(S110)
137-137: Do not catch blind exception: Exception
(BLE001)
tests/test_telemetry.py
127-127: Unused lambda argument: data
(ARG005)
132-132: Possible hardcoded password assigned to argument: "token"
(S106)
138-138: Possible hardcoded password assigned to: "token"
(S105)
273-273: Unused lambda argument: data
(ARG005)
385-385: Unused lambda argument: data
(ARG005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test 3.14
🔇 Additional comments (14)
docs/configuration.md (3)
80-82: LGTM! Clear documentation of telemetry isolation.The updated description accurately reflects the new session-level hook approach, making it clear to users that enabling telemetry won't affect other HTTP clients in their application.
112-129: Good example for custom callback usage.The example clearly demonstrates how to route telemetry data to a custom destination. The documented fields (method, url, status_code, request_headers, request_body, response_headers, response_body) match the implementation in
telemetry.py_response_hook.
131-142: Useful guidance for existing logfire users.This section addresses a common use case where users already have logfire configured in their application. The example correctly shows that no token is needed when relying on existing configuration.
src/garth/telemetry.py (3)
108-138: LGTM! Response hook implementation is solid.The
*args, **kwargsparameters (flagged by static analysis) are necessary for compatibility with therequests.Sessionhook signature. The silent exception handling aligns with the PR objective that telemetry errors should never break the application.One minor observation:
response.texton line 133 could theoretically raise an exception for certain encoding issues, but it's safely wrapped in the try/except block.
193-210: LGTM! Logfire configuration logic is correct.The early return when no token is available (lines 198-200) correctly handles the case where users have their own logfire configuration. When
_logfire_configuredremainsFalse, the default callback will still work if the user has already configured logfire externally.
212-223: LGTM! Idempotent session attachment.The
attachmethod correctly prevents duplicate hook registration. This is important sinceClient.configure()can be called multiple times.src/garth/http.py (2)
68-68: LGTM! New callback parameter integrated correctly.The
telemetry_callbackparameter is properly typed and follows the existing pattern for telemetry configuration options.
105-112: LGTM! Telemetry configuration and attachment flow is correct.The flow correctly:
- Configures telemetry settings (including the optional callback)
- Attaches the telemetry hooks to the session
Since
Telemetry.attach()is idempotent, calling it on everyconfigure()invocation is safe and ensures the hooks are always attached when telemetry is enabled.tests/test_telemetry.py (6)
69-95: LGTM! Well-structured VCR test with callback capture.The test correctly uses
@pytest.mark.vcrdecorator as per coding guidelines, and the callback capture pattern cleanly verifies telemetry data without requiring logfire mocking.
181-207: LGTM! Comprehensive response hook test.The test properly verifies:
- Data capture via callback
- Request/response metadata extraction
- Sanitization of sensitive data (password in request, token in response)
269-286: LGTM! Exception handling test verifies resilience.This test confirms that telemetry errors (simulated via
ZeroDivisionError) don't propagate and break the application.
345-371: LGTM! Idempotency tests are thorough.Good coverage for both single attach and multiple attach scenarios. This validates the
_attached_sessionstracking mechanism works correctly.
374-409: LGTM! Tests properly verify callback/logfire interaction.These tests confirm:
- Custom callback skips logfire configuration (lines 374-390)
- Missing token skips logfire configuration (lines 393-409)
Both are important edge cases for the PR's isolation objectives.
412-426: LGTM! Client integration test validates end-to-end flow.This test confirms that
Client.configure()properly passes thetelemetry_callbackthrough to the underlyingTelemetryinstance.
matin
force-pushed
the
issue-181-isolated-telemetry
branch
4 times, most recently
from
2afebdd to
8993362
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/garth/telemetry.py:
- Around line 174-181: Add a unit test that sets Telemetry.enabled=True with no
LOGFIRE_TOKEN and no custom callback, ensures after init that
telemetry._configured is True and telemetry._logfire_configured is False, then
patch the global "logfire" module with a mock and call
telemetry._response_hook(dummy_request, dummy_response); assert the call does
not raise and that mock_logfire.info was not called (verifying the default
callback behaves safely when logfire remains unconfigured). Use unique symbols
Telemetry, _configure_logfire, _response_hook, _default_callback,
_logfire_configured and _configured to locate code under test.
🧹 Nitpick comments (3)
src/garth/telemetry.py (2)
87-90: Minor: Consider memory leak potential with_attached_sessions.The
_attached_sessionsset stores session IDs indefinitely. If sessions are created and destroyed frequently, this set will grow unbounded. Additionally, if a session is garbage collected and a new session happens to get the sameid(), the hook won't be attached to the new session.For typical usage (single long-lived session per
Client), this is unlikely to be an issue. If high-frequency session creation becomes a use case, consider usingweakref.WeakSetto track sessions or clearing the set when sessions are replaced.
98-128: Consider logging telemetry exceptions for debugging.The broad exception handler at lines 127-128 silently swallows all errors. While this is intentional to prevent telemetry from breaking the application, completely silent failures can make debugging difficult.
Consider logging at DEBUG level or to stderr when exceptions occur:
♻️ Suggested improvement
callback = self.callback or self._default_callback callback(data) - except Exception: - pass # Don't let telemetry errors break the app + except Exception as e: + import logging + logging.getLogger(__name__).debug( + "Telemetry hook error: %s", e, exc_info=True + )Note: The unused
*args, **kwargsparameters are required by the requests hook signature and can be safely ignored.docs/telemetry.md (1)
55-66: Documentation is accurate; consider adding clarity about why garth skips logfire configuration.The example correctly demonstrates using garth with a pre-configured logfire setup. When
garth.configure(telemetry_enabled=True)is called without a token,_configure_logfire()returns early (line 190 oftelemetry.py), allowing garth to use the existing logfire configuration that was already initialized by the user'slogfire.configure()call. Recent logfire releases require explicitconfigure()before callinglogfire.info(), and the documentation example shows the correct ordering, so telemetry will work as documented.For improved clarity, consider adding a note explaining that garth deliberately skips its own logfire initialization in this scenario and relies on the application's pre-existing logfire configuration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/configuration.mddocs/telemetry.mdmkdocs.ymlsrc/garth/http.pysrc/garth/sso.pysrc/garth/telemetry.pytests/test_telemetry.py
💤 Files with no reviewable changes (1)
- docs/configuration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/garth/http.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest with VCR cassettes for HTTP recording/playback in tests
Mirror test directory structure to match source code structure
Files:
tests/test_telemetry.py
tests/**
⚙️ CodeRabbit configuration file
tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
Files:
tests/test_telemetry.py
🧠 Learnings (1)
📚 Learning: 2026-01-09T22:41:07.977Z
Learnt from: CR
Repo: matin/garth PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T22:41:07.977Z
Learning: Applies to tests/**/*.py : Use pytest with VCR cassettes for HTTP recording/playback in tests
Applied to files:
tests/test_telemetry.py
🧬 Code graph analysis (2)
tests/test_telemetry.py (2)
src/garth/http.py (1)
configure(50-112)src/garth/telemetry.py (4)
configure(130-181)_response_hook(98-128)_scrubbing_callback(76-78)attach(202-213)
src/garth/telemetry.py (1)
src/garth/http.py (1)
configure(50-112)
🪛 LanguageTool
docs/telemetry.md
[grammar] ~55-~55: Ensure spelling is correct
Context: ...callback instead. ## Use with existing logfire setup If your application already conf...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~55-~55: Ensure spelling is correct
Context: ...ead. ## Use with existing logfire setup If your application already configures l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.14.10)
tests/test_telemetry.py
127-127: Unused lambda argument: data
(ARG005)
132-132: Possible hardcoded password assigned to argument: "token"
(S106)
138-138: Possible hardcoded password assigned to: "token"
(S105)
273-273: Unused lambda argument: data
(ARG005)
376-376: Unused lambda argument: data
(ARG005)
src/garth/telemetry.py
98-98: Unused method argument: args
(ARG002)
98-98: Unused method argument: kwargs
(ARG002)
127-128: try-except-pass detected, consider logging the exception
(S110)
127-127: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (13)
src/garth/sso.py (1)
48-52: LGTM! Hook propagation ensures telemetry coverage for OAuth flows.Correctly extends the child session's response hooks with the parent's, ensuring telemetry hooks attached to the main
Client.sessare inherited byGarminOAuth1Sessioninstances used inget_oauth1_tokenandexchange.src/garth/telemetry.py (4)
4-7: LGTM!Imports are appropriate for the new session-based telemetry implementation.
76-78: LGTM!Correctly bypasses logfire's scrubbing since data is already sanitized in
_response_hookbefore reaching the callback.
183-200: LGTM!Logfire configuration is properly guarded and only runs when using the default callback with a valid token.
202-213: LGTM!The
attachmethod correctly implements idempotent hook attachment using session identity tracking.docs/telemetry.md (1)
1-96: LGTM! Comprehensive telemetry documentation.Documentation clearly covers all configuration options, use cases, and security considerations (redaction). The static analysis hints about "logfire" are false positives—it's a product name.
mkdocs.yml (1)
44-44: LGTM!Navigation entry correctly added for the new telemetry documentation.
tests/test_telemetry.py (6)
1-14: LGTM!Imports are appropriate for testing the new telemetry functionality.
47-54: LGTM!Test correctly verifies the new
callbackfield defaults toNone.
69-96: LGTM! Good VCR-based integration test.Correctly uses
@pytest.mark.vcrfor HTTP recording/playback and verifies telemetry data capture through the callback mechanism.
181-287: LGTM! Comprehensive response hook test coverage.Tests cover all key scenarios: string body, bytes body, missing body, disabled state, and exception handling. The static analysis warnings about unused lambda arguments are false positives—the lambdas are intentionally minimal for testing purposes.
336-417: LGTM! Good test coverage for attach and callback functionality.Tests properly verify:
- Hook attachment to sessions
- Idempotent attachment behavior
- Custom callback skipping logfire configuration
- Missing token behavior
- Client integration with telemetry callback
125-139: LGTM!Test correctly verifies all
configureparameters including the newcallback. The static analysis warnings about hardcoded tokens are false positives—these are intentional test values.
| if not self.enabled: | ||
| return | ||
|
|
||
| # Set token if provided | ||
| # Configure logfire only if using default callback and not yet done | ||
| if self.callback is None and not self._logfire_configured: | ||
| self._configure_logfire() | ||
|
|
||
| self._configured = True |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the telemetry.py file around the mentioned lines
cd /root && find . -name "telemetry.py" -type fRepository: matin/garth
Length of output: 110
🏁 Script executed:
# Let's look at the configure method and related code
head -200 src/garth/telemetry.py | tail -100Repository: matin/garth
Length of output: 3545
🏁 Script executed:
# Check the _default_callback implementation
rg -n "_default_callback" src/garth/telemetry.py -A 5Repository: matin/garth
Length of output: 486
🏁 Script executed:
# Check the _configure_logfire implementation
rg -n "_configure_logfire" src/garth/telemetry.py -A 10Repository: matin/garth
Length of output: 527
🏁 Script executed:
# Check _response_hook to see how callbacks are used
rg -n "_response_hook" src/garth/telemetry.py -A 10Repository: matin/garth
Length of output: 601
🏁 Script executed:
# Check test coverage as mentioned in the review
rg -n "enabled.*True" tests/test_telemetry.py | head -30Repository: matin/garth
Length of output: 570
🏁 Script executed:
# Let's examine the test file more thoroughly to see if this edge case is tested
cat tests/test_telemetry.pyRepository: matin/garth
Length of output: 12551
🏁 Script executed:
# Let's check if there are any tests that specifically test enabled=True without token
rg -n "enabled.*True" tests/test_telemetry.py -B 3 -A 5 | head -50Repository: matin/garth
Length of output: 1710
🏁 Script executed:
# Let's look for tests that don't set a token
rg -n "configure\(" tests/test_telemetry.py -A 3 | head -40Repository: matin/garth
Length of output: 1025
🏁 Script executed:
# Check how logfire behaves when not configured - search for any error handling
rg -n "logfire" src/garth/telemetry.py -B 2 -A 2Repository: matin/garth
Length of output: 3066
🏁 Script executed:
# Let's verify what logfire's default behavior is when info() is called unconfigured
rg -n "def _default_callback" src/garth/telemetry.py -A 5Repository: matin/garth
Length of output: 262
🏁 Script executed:
# Check if there's a test that actually invokes _response_hook when logfire is unconfigured
rg -n "test_telemetry_no_token_skips_logfire_config" tests/test_telemetry.py -A 20Repository: matin/garth
Length of output: 805
🏁 Script executed:
# Search for any explicit logfire initialization or error handling in default callback
rg -n "logfire" src/garth/telemetry.pyRepository: matin/garth
Length of output: 1316
Add test coverage for enabled telemetry without token or callback.
The edge case exists: when enabled=True without a LOGFIRE_TOKEN or custom callback, _configure_logfire() returns early without setting _logfire_configured=True (line 190). However, _configured is still set to True at line 181.
When _response_hook subsequently fires, it calls _default_callback, which imports logfire and calls logfire.info() on an unconfigured instance. This produces warnings or fails silently depending on logfire's behavior. The exception handler in _response_hook (lines 127-128) only catches exceptions, not warnings.
Existing test test_telemetry_no_token_skips_logfire_config verifies this state can exist but does not test actual hook invocation in this scenario. Add a test that mocks logfire and invokes _response_hook to verify behavior when logfire is unconfigured.
🤖 Prompt for AI Agents
In @src/garth/telemetry.py around lines 174 - 181, Add a unit test that sets
Telemetry.enabled=True with no LOGFIRE_TOKEN and no custom callback, ensures
after init that telemetry._configured is True and telemetry._logfire_configured
is False, then patch the global "logfire" module with a mock and call
telemetry._response_hook(dummy_request, dummy_response); assert the call does
not raise and that mock_logfire.info was not called (verifying the default
callback behaves safely when logfire remains unconfigured). Use unique symbols
Telemetry, _configure_logfire, _response_hook, _default_callback,
_logfire_configured and _configured to locate code under test.
8993362 to
8ab9506
Compare
Previously, telemetry used logfire.instrument_requests() which globally patches all requests.Session usage. This could conflict with applications that have their own logfire/OpenTelemetry setup. Now telemetry uses session-level hooks that only capture garth's requests: - Attach response hooks to garth's specific Session instance - Only garth's HTTP requests are logged - No global monkey-patching of the requests library - Custom callback support for alternative telemetry destinations - Logfire cloud remains the default when no callback is provided Fixes #181 Co-Authored-By: Claude Opus 4.5 <[email protected]>
matin
force-pushed
the
issue-181-isolated-telemetry
branch
from
8ab9506 to
57a7a98
Compare
1 participant
Summary
logfire.instrument_requests()with session-level hookstelemetry_callbackparameter for custom telemetry destinationsChanges
telemetry.py: Rewritten to usesession.hooks['response']instead of global instrumentationhttp.py: Addedtelemetry_callbackparameter, calltelemetry.attach(session)docs/configuration.md: Updated with custom callback examples and isolation notesWhy
The previous implementation had two issues:
logfire.configure()set up the global OpenTelemetry provider, potentially overwriting user's configlogfire.instrument_requests()patched ALL requests globally, not just garth'sNow telemetry is completely isolated to garth's session.
Fixes #181
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.