Conversation
- Add telemetry module with Telemetry class - Integrate telemetry into Client.configure() - Support GARTH_TELEMETRY, GARTH_TELEMETRY_SERVICE_NAME env vars - Sanitize sensitive data (passwords, tokens) from logs - Log sanitized response body on auth failures for debugging - Disabled by default, can be enabled via code or env vars - Add logfire dependency - Add telemetry documentation Co-Authored-By: Claude Opus 4.5 <[email protected]>
WalkthroughAdds opt-in telemetry: new Telemetry module with sanitization and lazy Logfire instrumentation, HTTP client wiring/config options, pyproject dependency addition, docs update, and new tests; no breaking API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Telemetry as Telemetry
participant Logfire as Logfire
participant HTTP as HTTP Request/Response
Client->>Telemetry: instantiate Telemetry
Client->>Telemetry: configure(enabled, service_name, send_to_logfire, token)
Telemetry->>Telemetry: apply env var overrides
Telemetry->>Logfire: initialize/configure instrumentation & scrubbing (lazy)
Client->>HTTP: send request
HTTP->>HTTP: perform request / receive response
HTTP->>Telemetry: invoke _response_hook(span, request, response)
Telemetry->>Telemetry: sanitize headers/body/cookies
Telemetry->>Logfire: attach sanitized attributes to span
Logfire->>Logfire: apply scrubbing callback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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 #178 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 66 68 +2
Lines 3118 3381 +263
==========================================
+ Hits 3118 3381 +263
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:
|
- Log sanitized request headers, request body, response headers, and response body - Use VCR cassette test instead of mock for telemetry integration test - Make sanitization consistent with VCR conftest (use SANITIZED, same patterns) - Add scrubbing callback to bypass Logfire's default scrubbing for pre-sanitized attributes - Add logfire[requests] extra for OpenTelemetry requests instrumentation Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Move sanitization logic to telemetry.py as the single source of truth - Export sanitize(), sanitize_cookie(), sanitize_headers(), and REDACTED - Update conftest.py to import and use shared sanitization functions - Change from SANITIZED to [REDACTED] for consistency - Remove duplicate sanitization code from conftest.py Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
![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 (4)
pyproject.toml (1)
12-12: Consider makinglogfirean optional dependency.The PR objectives state telemetry is opt-in and disabled by default, but
logfire[requests]is added as a required dependency. This means all users will install logfire even if they never enable telemetry, adding unnecessary overhead and dependency surface.Consider moving it to
[project.optional-dependencies]:♻️ Suggested refactor
dependencies = [ "requests>=2.0.0,<3.0.0", "pydantic>=1.10.12,<3.0.0", "requests-oauthlib>=1.3.1,<3.0.0", - "logfire[requests]>=2.0", ] [project.optional-dependencies] +telemetry = [ + "logfire[requests]>=2.0", +] docs = [ "mkdocs>=1.6", "mkdocs-material>=9.5", ]This would require updating
src/garth/telemetry.pyto handle the import gracefully when logfire is not installed.tests/test_telemetry.py (1)
82-88: Consider clearingLOGFIRE_SEND_TO_LOGFIREfor test isolation.Other env var tests (lines 91-98) explicitly clear unrelated env vars. For consistency and isolation:
♻️ Suggested improvement
def test_telemetry_env_service_name(monkeypatch): monkeypatch.setenv("GARTH_TELEMETRY_SERVICE_NAME", "custom-service") monkeypatch.delenv("GARTH_TELEMETRY", raising=False) + monkeypatch.delenv("LOGFIRE_SEND_TO_LOGFIRE", raising=False) t = Telemetry() t.configure() assert t.service_name == "custom-service"src/garth/telemetry.py (2)
63-71: Minor inefficiency in case-insensitive header matching.The current implementation creates a new lowercase list for each key. Consider a more efficient approach:
♻️ Suggested optimization
+SENSITIVE_HEADERS_LOWER = {h.lower() for h in SENSITIVE_HEADERS} + def sanitize_headers(headers: dict) -> dict: """Sanitize sensitive headers.""" sanitized = dict(headers) for key in list(sanitized.keys()): - if key in SENSITIVE_HEADERS or key.lower() in [ - h.lower() for h in SENSITIVE_HEADERS - ]: + if key in SENSITIVE_HEADERS or key.lower() in SENSITIVE_HEADERS_LOWER: sanitized[key] = REDACTED return sanitized
74-104: Intentional silent exception handling is acceptable here.The static analysis hints flag
try-except-passblocks (S110/BLE001). In this context, silently catching exceptions is the correct behavior—telemetry hooks must never break HTTP requests. A failure to set a span attribute should not propagate.Consider adding a brief comment to clarify intent:
♻️ Suggested documentation
def _response_hook(span, request, response): - """Log sanitized request/response details for debugging.""" + """Log sanitized request/response details for debugging. + + Exceptions are silently caught to ensure telemetry never breaks requests. + """ try:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/cassettes/test_telemetry_enabled_request.yamlis excluded by!tests/**/cassettes/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
docs/configuration.mdpyproject.tomlsrc/garth/http.pysrc/garth/telemetry.pytests/conftest.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.pytests/conftest.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.pytests/conftest.py
🧠 Learnings (2)
📚 Learning: 2026-01-09T22:41:07.978Z
Learnt from: CR
Repo: matin/garth PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T22:41:07.978Z
Learning: Expose main client instance as `garth.client` and provide `connectapi()` method for direct API calls returning JSON
Applied to files:
docs/configuration.md
📚 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/conftest.py
🧬 Code graph analysis (4)
src/garth/http.py (1)
src/garth/telemetry.py (2)
Telemetry(115-176)configure(122-176)
src/garth/telemetry.py (1)
src/garth/http.py (2)
request(145-184)configure(49-108)
tests/test_telemetry.py (3)
src/garth/http.py (2)
Client(20-280)configure(49-108)src/garth/telemetry.py (3)
Telemetry(115-176)sanitize(41-60)configure(122-176)tests/conftest.py (2)
vcr(83-86)authed_client(67-79)
tests/conftest.py (1)
src/garth/telemetry.py (2)
sanitize(41-60)sanitize_cookie(36-38)
🪛 Ruff (0.14.10)
src/garth/telemetry.py
79-80: try-except-pass detected, consider logging the exception
(S110)
79-79: Do not catch blind exception: Exception
(BLE001)
88-89: try-except-pass detected, consider logging the exception
(S110)
88-88: Do not catch blind exception: Exception
(BLE001)
96-97: try-except-pass detected, consider logging the exception
(S110)
96-96: Do not catch blind exception: Exception
(BLE001)
103-104: try-except-pass detected, consider logging the exception
(S110)
103-103: Do not catch blind exception: Exception
(BLE001)
tests/test_telemetry.py
107-107: Possible hardcoded password assigned to argument: "token"
(S106)
112-112: Possible hardcoded password assigned to: "token"
(S105)
🔇 Additional comments (11)
docs/configuration.md (1)
76-135: LGTM!The telemetry documentation is comprehensive, covering environment variables, programmatic configuration, OTLP backend alternatives, and clearly documenting what data is logged versus redacted.
tests/conftest.py (1)
11-11: LGTM!Good consolidation of sanitization logic. Importing from
garth.telemetryeliminates duplicate code and ensures consistent redaction behavior between runtime telemetry and VCR cassette recording.Also applies to: 96-96, 132-132, 140-140
src/garth/http.py (2)
34-34: LGTM!Clean integration of telemetry into the Client class. The Telemetry instance is properly initialized before
configure()is called, and the_configuredflag inTelemetry.configure()ensures idempotent behavior.Also applies to: 39-39
63-66: LGTM!The telemetry configuration parameters are properly threaded through to
Telemetry.configure(). The parameter naming is clear and follows the existing pattern in the method.Also applies to: 103-108
tests/test_telemetry.py (4)
7-34: LGTM!Comprehensive sanitization tests covering JSON fields (password, access_token, refresh_token, oauth tokens) and query parameter patterns. Good assertions that verify both removal of sensitive values and presence of redaction markers.
37-55: LGTM!Good coverage of Telemetry defaults and environment variable override behavior. The tests verify that environment variables take precedence over programmatic configuration, which aligns with the documented behavior.
Also applies to: 74-98
58-71: LGTM!Good integration test using VCR cassette recording as per coding guidelines. Setting
send_to_logfire=Falseis a practical approach to test instrumentation without requiring Logfire credentials.
101-125: LGTM!Good tests for full parameter configuration and idempotent behavior. The static analysis hints about hardcoded passwords (S105/S106) are false positives—using
"test-token"in test code is appropriate.src/garth/telemetry.py (3)
7-7: LGTM!Well-organized constants for sensitive data patterns. The separation between query param patterns, JSON fields, and headers provides clear categorization of what gets redacted.
Also applies to: 10-18, 21-30, 33-33
36-60: LGTM with a note on nested JSON handling.The sanitization logic is solid for the common use cases. Note that nested JSON structures (e.g.,
{"user": {"password": "secret"}}) won't have inner sensitive fields redacted—only top-level dict keys are checked. This is likely acceptable given the expected API payload shapes, but worth keeping in mind if deeper structures are encountered.
114-176: LGTM!The Telemetry dataclass is well-designed:
- Disabled by default (satisfies PR objective)
- Single
configure()call to enable (satisfies single-line toggle objective)_configuredflag ensures idempotent configuration- Lazy import of
logfireavoids import overhead when disabled- Environment variable overrides are applied correctly after programmatic config
One consideration: if
logfirebecomes an optional dependency per the earlier suggestion, Line 167 would need atry-except ImportErrorwith a helpful error message.
- Test sanitize_cookie and sanitize_headers functions - Test _response_hook with string body, bytes body, no body, and exceptions - Test _scrubbing_callback for allowed and default scrubbing - Test environment variable GARTH_TELEMETRY=true enables telemetry - Test token parameter sets LOGFIRE_TOKEN env var Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_telemetry.py (1)
274-287: Consider verifying the environment variable side effect.The test verifies that
t.tokenis set, but theconfiguremethod also setsos.environ["LOGFIRE_TOKEN"]when a token is provided and telemetry is enabled. Adding an assertion for this would make the test more complete.🔧 Suggested assertion
t = Telemetry() t.configure(token="my-test-token") assert t.token == "my-test-token" assert t._configured is True + import os + assert os.environ.get("LOGFIRE_TOKEN") == "my-test-token"src/garth/telemetry.py (3)
63-71: Minor optimization opportunity (optional).The list comprehension on line 68 is recreated for each header key. For the current small set of headers this is fine, but a precomputed lowercase set would be more efficient.
🔧 Suggested optimization
+# At module level +SENSITIVE_HEADERS_LOWER = {h.lower() for h in SENSITIVE_HEADERS} + def sanitize_headers(headers: dict) -> dict: """Sanitize sensitive headers.""" sanitized = dict(headers) for key in list(sanitized.keys()): - if key in SENSITIVE_HEADERS or key.lower() in [ - h.lower() for h in SENSITIVE_HEADERS - ]: + if key in SENSITIVE_HEADERS or key.lower() in SENSITIVE_HEADERS_LOWER: sanitized[key] = REDACTED return sanitized
74-100: Consider adding debug logging for swallowed exceptions.The silent exception handling is appropriate for observability code that shouldn't break the application. However, completely swallowing exceptions makes it difficult to troubleshoot telemetry configuration issues. Consider logging at DEBUG level.
🔧 Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + def _response_hook(span, request, response): """Log sanitized request/response details for debugging.""" try: req_headers = sanitize_headers(dict(request.headers)) span.set_attribute("http.request.headers", str(req_headers)) - except Exception: - pass + except Exception as e: + logger.debug("Failed to set request headers attribute: %s", e) # ... similar for other try blocks
103-112: Add defensive bounds check for path access.The callback accesses
m.path[0]andm.path[1]without verifying the path length. While Logfire's API likely guarantees a valid path, defensive coding would prevent potential IndexError.🔧 Suggested fix
def _scrubbing_callback(m): """Allow our pre-sanitized attributes through Logfire's scrubbing.""" - if m.path[0] == "attributes" and m.path[1] in ( + if len(m.path) >= 2 and m.path[0] == "attributes" and m.path[1] in ( "http.request.headers", "http.request.body", "http.response.headers", "http.response.body", ): return m.value return None # Use default scrubbing
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/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
🧬 Code graph analysis (2)
src/garth/telemetry.py (1)
src/garth/http.py (1)
configure(49-108)
tests/test_telemetry.py (2)
src/garth/http.py (2)
configure(49-108)request(145-184)src/garth/telemetry.py (7)
Telemetry(116-177)_response_hook(74-100)_scrubbing_callback(103-112)sanitize(41-60)sanitize_cookie(36-38)sanitize_headers(63-71)configure(123-177)
🪛 Ruff (0.14.10)
src/garth/telemetry.py
79-80: try-except-pass detected, consider logging the exception
(S110)
79-79: Do not catch blind exception: Exception
(BLE001)
88-89: try-except-pass detected, consider logging the exception
(S110)
88-88: Do not catch blind exception: Exception
(BLE001)
94-95: try-except-pass detected, consider logging the exception
(S110)
94-94: Do not catch blind exception: Exception
(BLE001)
99-100: try-except-pass detected, consider logging the exception
(S110)
99-99: Do not catch blind exception: Exception
(BLE001)
tests/test_telemetry.py
117-117: Possible hardcoded password assigned to argument: "token"
(S106)
122-122: Possible hardcoded password assigned to: "token"
(S105)
285-285: Possible hardcoded password assigned to argument: "token"
(S106)
287-287: Possible hardcoded password assigned to: "token"
(S105)
🔇 Additional comments (9)
tests/test_telemetry.py (5)
1-44: LGTM!Imports are correct and the sanitization tests provide good coverage for passwords, tokens, OAuth tokens, and query parameters. The assertions verify both that sensitive data is removed and that the redaction marker is present.
47-135: LGTM!Good coverage of Telemetry configuration scenarios including defaults, disabled state, environment variable overrides, and the single-configuration guard. The VCR-recorded test properly exercises telemetry with real API requests while avoiding the need for Logfire credentials.
138-161: LGTM!The cookie and header sanitization tests cover both the standard case and case-insensitive matching for headers, which is important since HTTP headers are case-insensitive.
164-233: LGTM!Excellent coverage of the response hook including string bodies, bytes bodies, no body, and exception handling. The tests verify both the attribute count and that sensitive data is properly sanitized from the captured values.
236-251: LGTM!The scrubbing callback tests correctly verify that pre-sanitized HTTP attributes are passed through while other attributes fall back to Logfire's default scrubbing behavior.
src/garth/telemetry.py (4)
1-33: LGTM!The constants are well-organized and provide comprehensive coverage of sensitive data patterns for OAuth tokens, passwords, and authorization headers. The separation into query params, JSON fields, and headers allows for targeted sanitization in each context.
36-38: LGTM!The aggressive approach of redacting all cookie
key=valuepairs (including attributes likepathanddomain) is appropriate for telemetry logging where minimizing data exposure is preferred over preserving non-sensitive metadata.
41-60: LGTM!The two-phase approach (query params first, then JSON parsing) handles the common formats. The fallback to returning the original text when JSON parsing fails ensures non-JSON content is still returned after query param sanitization.
115-177: LGTM!The Telemetry class design is solid:
- Disabled by default (opt-in) as per requirements
- Environment variables properly override programmatic settings
- Lazy import of
logfireavoids overhead when telemetry is disabled- Single-configuration guard prevents duplicate instrumentation
- Scrubbing callback integration ensures pre-sanitized data passes through
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/garth/telemetry.py:
- Around line 102-111: The callback _scrubbing_callback can raise IndexError
when m.path has fewer than two elements; add a guard that checks the length of
m.path (e.g., len(m.path) >= 2) or ensures m.path has at least two components
before accessing m.path[0] and m.path[1]; only perform the existing comparisons
for "attributes" and the listed attribute names when the length check passes and
otherwise return None to preserve default scrubbing behavior.
- Line 175: The call to
logfire.instrument_requests(response_hook=_response_hook) can trigger
OpenTelemetry MeterProvider.get_meter() signature conflicts due to broad deps;
fix by pinning logfire and OpenTelemetry to known-compatible versions in your
dependency file (lock logfire to a 2.x release and match
opentelemetry-api/opentelemetry-sdk versions that you’ve verified work together)
and/or add a runtime compatibility guard: wrap logfire.instrument_requests(...)
in a try/except that catches TypeError/TypeError-like signature errors from
MeterProvider.get_meter, log a clear error referencing
logfire.instrument_requests and _response_hook, and skip instrumentation so the
app remains functional while you enforce compatible pins and add integration
tests to validate the combination.
🧹 Nitpick comments (3)
pyproject.toml (1)
12-12: Consider making logfire an optional dependency.Since telemetry is disabled by default and described as "optional" in the PR objectives, forcing all users to install
logfire[requests]increases package size and dependencies unnecessarily. Consider moving it to[project.optional-dependencies](e.g., under atelemetryextra) and importing conditionally.[project.optional-dependencies] telemetry = [ "logfire[requests]>=2.0,<3.0", ]Then install with
pip install garth[telemetry]when needed.src/garth/telemetry.py (2)
73-99: Consider narrowing exception handling or adding debug logging.The broad
except Exception: passblocks ensure telemetry never crashes the main application, which is appropriate for production. However, this can hide bugs during development. Consider catching specific exceptions (e.g.,AttributeError,UnicodeDecodeError) or adding conditional debug logging.
163-165: Modifyingos.environhas process-wide side effects.Setting
os.environ["LOGFIRE_TOKEN"]persists for the entire process lifetime and affects any other code reading this environment variable. Consider documenting this behavior or using logfire's API to pass the token directly if supported.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/configuration.mdpyproject.tomlsrc/garth/telemetry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/configuration.md
🧰 Additional context used
🪛 GitHub Actions: CI
src/garth/telemetry.py
[error] 175-175: Telemetry configuration failed: MeterProvider.get_meter() got multiple values for argument 'version'.
🪛 Ruff (0.14.10)
src/garth/telemetry.py
78-79: try-except-pass detected, consider logging the exception
(S110)
78-78: Do not catch blind exception: Exception
(BLE001)
87-88: try-except-pass detected, consider logging the exception
(S110)
87-87: Do not catch blind exception: Exception
(BLE001)
93-94: try-except-pass detected, consider logging the exception
(S110)
93-93: Do not catch blind exception: Exception
(BLE001)
98-99: try-except-pass detected, consider logging the exception
(S110)
98-98: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
src/garth/telemetry.py (4)
1-35: LGTM!Good organization of sanitization constants with precomputed lowercase set for efficient header matching.
37-39: LGTM!
51-59: JSON sanitization only handles top-level fields.Nested objects containing sensitive fields (e.g.,
{"user": {"password": "secret"}}) won't be sanitized. If Garmin responses never nest sensitive data, this is acceptable; otherwise, consider recursive sanitization.
147-158: Environment variables override programmatic settings.The current flow applies programmatic arguments first, then overwrites with environment variables. This means
client.configure(telemetry_enabled=False)can be overridden byGARTH_TELEMETRY=true. If this is intentional (env vars take precedence), document it; otherwise, reverse the order.
| def _scrubbing_callback(m): | ||
| """Allow our pre-sanitized attributes through Logfire's scrubbing.""" | ||
| if m.path[0] == "attributes" and m.path[1] in ( | ||
| "http.request.headers", | ||
| "http.request.body", | ||
| "http.response.headers", | ||
| "http.response.body", | ||
| ): | ||
| return m.value | ||
| return None # Use default scrubbing |
There was a problem hiding this comment.
Add bounds check on m.path to prevent IndexError.
If m.path has fewer than 2 elements, accessing m.path[1] raises IndexError.
Proposed fix
def _scrubbing_callback(m):
"""Allow our pre-sanitized attributes through Logfire's scrubbing."""
- if m.path[0] == "attributes" and m.path[1] in (
+ if len(m.path) >= 2 and m.path[0] == "attributes" and m.path[1] in (
"http.request.headers",
"http.request.body",
"http.response.headers",
"http.response.body",
):
return m.value
return None # Use default scrubbing🤖 Prompt for AI Agents
In @src/garth/telemetry.py around lines 102 - 111, The callback
_scrubbing_callback can raise IndexError when m.path has fewer than two
elements; add a guard that checks the length of m.path (e.g., len(m.path) >= 2)
or ensures m.path has at least two components before accessing m.path[0] and
m.path[1]; only perform the existing comparisons for "attributes" and the listed
attribute names when the length check passes and otherwise return None to
preserve default scrubbing behavior.
Addresses CodeRabbit review concern about potential version incompatibility with OpenTelemetry. logfire 2.11+ has compatible MeterProvider.get_meter() signatures. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @pyproject.toml:
- Line 12: Move the required "logfire[requests]>=2.11,<3.0" entry out of the
main dependencies and into [project.optional-dependencies] under a new
"telemetry" extra in pyproject.toml so users only install it when they opt-in;
then in src/garth/telemetry.py (at the import site around the current import on
line ~167) wrap the logfire import in a try/except ImportError and raise a clear
message instructing users to install the extra (e.g. "pip install
garth[telemetry]"); finally update the README/docs to document the telemetry
extra and how to enable telemetry via configure(enabled=True).
In @src/garth/telemetry.py:
- Around line 50-61: The current sanitization only replaces top-level keys;
update the sanitizer (the function that calls json.loads and iterates
JSON_SENSITIVE_FIELDS) to recursively walk parsed JSON structures: if the parsed
value is a dict, iterate its items and if a key is in JSON_SENSITIVE_FIELDS
replace its value with REDACTED, otherwise recurse into the value; if the value
is a list, recurse into each element; after recursion return json.dumps of the
sanitized structure. Use the existing JSON_SENSITIVE_FIELDS and REDACTED symbols
and ensure you still catch json.JSONDecodeError and TypeError and return the
original text when parsing fails.
🧹 Nitpick comments (2)
src/garth/telemetry.py (2)
73-99: Consider logging suppressed exceptions at debug level.The
except Exception: passblocks ensure telemetry never breaks the main flow, which is correct behavior. However, silently swallowing all exceptions makes debugging issues in the telemetry code difficult.Consider logging at debug level or narrowing to expected exception types (e.g.,
AttributeError,UnicodeDecodeError).♻️ Proposed improvement
+import logging + +logger = logging.getLogger(__name__) + def _response_hook(span, request, response): """Log sanitized request/response details for debugging.""" try: req_headers = sanitize_headers(dict(request.headers)) span.set_attribute("http.request.headers", str(req_headers)) - except Exception: - pass + except Exception as e: + logger.debug("Failed to set request headers attribute: %s", e) try: if request.body: body = request.body if isinstance(body, bytes): body = body.decode("utf-8", errors="replace") span.set_attribute("http.request.body", sanitize(body)) - except Exception: - pass + except Exception as e: + logger.debug("Failed to set request body attribute: %s", e) try: resp_headers = sanitize_headers(dict(response.headers)) span.set_attribute("http.response.headers", str(resp_headers)) - except Exception: - pass + except Exception as e: + logger.debug("Failed to set response headers attribute: %s", e) try: span.set_attribute("http.response.body", sanitize(response.text)) - except Exception: - pass + except Exception as e: + logger.debug("Failed to set response body attribute: %s", e)
164-165: SettingLOGFIRE_TOKENmodifies global process environment.Calling
os.environ["LOGFIRE_TOKEN"] = self.tokenaffects the entire process. If multipleTelemetryinstances are created with different tokens (unlikely but possible), they'll overwrite each other's tokens.Consider documenting this limitation or logging a warning if
LOGFIRE_TOKENis already set with a different value.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/cassettes/test_telemetry_enabled_request.yamlis excluded by!tests/**/cassettes/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
docs/configuration.mdpyproject.tomlsrc/garth/http.pysrc/garth/telemetry.pytests/conftest.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.pytests/conftest.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.pytests/conftest.py
🧠 Learnings (2)
📚 Learning: 2026-01-09T22:41:07.978Z
Learnt from: CR
Repo: matin/garth PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T22:41:07.978Z
Learning: Expose main client instance as `garth.client` and provide `connectapi()` method for direct API calls returning JSON
Applied to files:
docs/configuration.md
📚 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/conftest.py
🧬 Code graph analysis (4)
tests/test_telemetry.py (2)
src/garth/http.py (3)
configure(49-108)connectapi(220-226)request(145-184)src/garth/telemetry.py (6)
_response_hook(73-99)_scrubbing_callback(102-111)sanitize(42-61)sanitize_cookie(37-39)sanitize_headers(64-70)configure(122-176)
tests/conftest.py (2)
src/garth/telemetry.py (2)
sanitize(42-61)sanitize_cookie(37-39)src/garth/http.py (1)
request(145-184)
src/garth/http.py (1)
src/garth/telemetry.py (2)
Telemetry(115-176)configure(122-176)
src/garth/telemetry.py (1)
src/garth/http.py (2)
request(145-184)configure(49-108)
🪛 Ruff (0.14.10)
tests/test_telemetry.py
123-123: Possible hardcoded password assigned to argument: "token"
(S106)
128-128: Possible hardcoded password assigned to: "token"
(S105)
291-291: Possible hardcoded password assigned to argument: "token"
(S106)
293-293: Possible hardcoded password assigned to: "token"
(S105)
src/garth/telemetry.py
78-79: try-except-pass detected, consider logging the exception
(S110)
78-78: Do not catch blind exception: Exception
(BLE001)
87-88: try-except-pass detected, consider logging the exception
(S110)
87-87: Do not catch blind exception: Exception
(BLE001)
93-94: try-except-pass detected, consider logging the exception
(S110)
93-93: Do not catch blind exception: Exception
(BLE001)
98-99: try-except-pass detected, consider logging the exception
(S110)
98-98: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (21)
docs/configuration.md (1)
76-138: Documentation is clear and comprehensive.The telemetry section covers all configuration options, both environment variables and programmatic configuration. The examples are practical and the security-conscious note about redacted fields is helpful.
tests/conftest.py (3)
11-11: Good centralization of sanitization logic.Reusing the
sanitize,sanitize_cookie, andREDACTEDfromgarth.telemetryensures VCR cassettes use the same sanitization rules as production telemetry. This follows DRY principles and reduces the risk of inconsistencies.
89-102: Request sanitization refactored correctly.The
sanitize_requestfunction now uses the centralizedsanitize()andsanitize_cookie()functions, maintaining the same behavior while leveraging shared code.
137-143: VCR config updated consistently.Using
REDACTEDconstant for the Authorization header filter keeps the VCR cassettes consistent with other sanitization in the codebase.tests/test_telemetry.py (7)
1-14: Test imports and structure look good.The imports cover all the necessary components from the telemetry module, including the internal functions
_response_hookand_scrubbing_callbackfor thorough unit testing.
17-45: Sanitization tests provide good coverage.Tests cover password, access/refresh tokens, OAuth tokens, and query parameters. Each test verifies that sensitive values are removed and replaced with
[REDACTED].
68-88: VCR integration test is well-structured.The test properly mocks logfire to avoid OpenTelemetry conflicts in CI, configures telemetry with
send_to_logfire=False, and verifies a real API request works. Using@pytest.mark.vcraligns with the coding guidelines.
117-128: Static analysis false positives - safe to ignore.The Ruff S105/S106 warnings about "hardcoded passwords" at lines 123, 128 are false positives. These are test tokens (
"test-token") used to verify configuration, not actual secrets.
170-192: Response hook test verifies sanitization in span attributes.Good test that verifies all four attributes are set and sensitive data is sanitized in both request and response bodies.
228-239: Exception handling test ensures resilience.Good defensive test that verifies
_response_hooksilently handles exceptions without propagating them, which is important for telemetry code that shouldn't break the main request flow.
280-293: Token environment variable test is appropriate.Static analysis warnings (S105/S106) at lines 291, 293 are false positives. This test correctly verifies that the
tokenparameter results in setting theLOGFIRE_TOKENenvironment variable.src/garth/http.py (5)
13-13: Clean import of Telemetry class.The import is appropriately placed with other internal module imports.
34-39: Telemetry integration follows existing patterns.The
telemetryattribute is declared as a class-level type hint and instantiated in__init__, consistent with how other attributes likesessare handled.
63-66: Telemetry configuration parameters are well-designed.All four telemetry parameters are optional with
Nonedefaults, ensuring backward compatibility. The naming convention (telemetry_*prefix) clearly groups them together.
103-108: Telemetry configuration delegated appropriately.The
telemetry.configure()call is placed after the adapter mount, which is appropriate since telemetry configuration doesn't depend on HTTP adapter setup. The parameter mapping is straightforward.
17-17: User-Agent version updated.The User-Agent string was updated to
"GCM-iOS-5.19.1.2". If this is an intentional version bump to match current Garmin Connect Mobile versions, this is fine.src/garth/telemetry.py (5)
7-34: Sensitive data patterns look reasonable.The lists cover OAuth tokens, passwords, and other credentials. One minor question:
"embed"inQUERY_PARAM_PATTERNS(line 17) seems unusual among the other credential-related parameters. Is this intentionally redacted for privacy reasons, or is it an artifact that could be removed?
37-39: LGTM!The cookie sanitization correctly redacts all values while preserving the structure for debugging.
64-70: LGTM!Clean implementation with proper case-insensitive header matching and non-mutating behavior.
102-111: LGTM!The scrubbing callback correctly bypasses Logfire's default scrubbing for pre-sanitized attributes while preserving default behavior elsewhere.
160-176: Telemetry cannot be disabled once enabled after instrumentation.Once
logfire.instrument_requests()is called, the_response_hookcannot be disabled. The_configuredguard at line 160 prevents re-initialization, and_response_hookhas no access to theenabledstate (it's a module-level function without reference to the Telemetry instance). Settingself.enabled = Falsein subsequentconfigure()calls has no effect on logging.If one-time initialization is intentional, document this in the
configure()docstring. Otherwise, consider refactoring_response_hookto check the enabled state—either by wrapping it in a closure that captures the instance, or by checking a global/thread-local state variable.
| "requests>=2.0.0,<3.0.0", | ||
| "pydantic>=1.10.12,<3.0.0", | ||
| "requests-oauthlib>=1.3.1,<3.0.0", | ||
| "logfire[requests]>=2.11,<3.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "pyproject.toml" -type f 2>/dev/null | head -5Repository: matin/garth
Length of output: 37
🏁 Script executed:
cat pyproject.toml | head -50Repository: matin/garth
Length of output: 1523
🏁 Script executed:
# Search for telemetry-related files
find . -name "*telemetry*" -type fRepository: matin/garth
Length of output: 159
🏁 Script executed:
# Look for where telemetry is configured/used
rg -l "telemetry" --type py | head -10Repository: matin/garth
Length of output: 137
🏁 Script executed:
# Check the package name
rg "name.*=" pyproject.toml | head -3Repository: matin/garth
Length of output: 126
🌐 Web query:
best practices optional dependencies Python packages poetry pip
💡 Result:
Summary — best practices for optional dependencies (Poetry / pip / setuptools / PEPs)
- Use extras (PEP 508 / project.optional-dependencies) for feature-based optional deps; declare extras in pyproject.toml (project.optional-dependencies / tool.poetry.extras). (peps.python.org)
- Name extras semantically (e.g., "docs", "postgres", "s3") and keep lists small and focused so users can opt into individual features. (setuptools.pypa.io)
- For libraries: prefer extras so downstream projects can opt-in (pip install package[extra]). For applications: prefer dependency groups or install-time options (apps often pin final environment). (python-poetry.org)
- Declare optional packages with optional=true (Poetry) and include them in an extras mapping; extras must refer to dependencies already listed. Test that extras resolve together. (python-poetry.org)
- Use environment markers (python/platform) for conditional optional deps and mark extras with extra == "" where needed. (python-poetry.org)
- Prefer PEP-compliant metadata (PEP 621/PEP 631) in pyproject.toml so tools (pip, setuptools, Poetry) interoperate. (peps.python.org)
- Document extras in README (what each extra enables and when to use it) and provide install examples (poetry install --extras / pip install package[extra]). (python-poetry.org)
- Avoid making a package unusable without extras; if an extra is required for functionality, either make it required or use PEP 771 default-optional-dependency-keys to provide sensible defaults. (peps.python.org)
- Keep extras stable (don’t rename/remove keys) — if you must remove an extra, keep an empty key to avoid breaking consumers (setuptools tip). Pin versions for heavy extras to reduce conflicts. (setuptools.pypa.io)
- Test combinations and CI matrix: run installs with no extras, each extra, and common combos to catch incompatibilities and re-resolve issues (Poetry’s re-resolve behavior can affect combos). (python-poetry.org)
If you want, I can:
- show exact pyproject.toml examples for Poetry (PEP 621 style) and pip-install commands; or
- produce a short CI matrix template to test extras.
Citations:
- 1: https://peps.python.org/pep-0631/?utm_source=openai
- 2: https://setuptools.pypa.io/en/stable/userguide/dependency_management.html?utm_source=openai
- 3: https://python-poetry.org/docs/cli/?utm_source=openai
- 4: https://python-poetry.org/docs/pyproject/?utm_source=openai
- 5: https://python-poetry.org/docs/main/dependency-specification/?utm_source=openai
- 6: https://peps.python.org/pep-0621/?utm_source=openai
- 7: https://python-poetry.org/docs/1.8/pyproject/?utm_source=openai
- 8: https://peps.python.org/pep-0771/?utm_source=openai
- 9: https://setuptools.pypa.io/en/stable/userguide/dependency_management.html?utm_source=openai
- 10: https://python-poetry.org/docs/main/dependency-specification/?utm_source=openai
🏁 Script executed:
cat -n src/garth/telemetry.pyRepository: matin/garth
Length of output: 6309
🏁 Script executed:
cat -n src/garth/http.pyRepository: matin/garth
Length of output: 11485
🏁 Script executed:
head -50 tests/test_telemetry.pyRepository: matin/garth
Length of output: 1284
🏁 Script executed:
# Check if there are any environment variables or config related to telemetry defaults
rg "telemetry\s*=" src/ --type py -A 2Repository: matin/garth
Length of output: 203
🏁 Script executed:
# Search for logfire imports to see if they're guarded
rg "import logfire" src/ --type py -B 2 -A 2Repository: matin/garth
Length of output: 274
Move logfire to optional dependencies.
Since telemetry is disabled by default and only imports logfire when explicitly enabled via configure(enabled=True), adding logfire[requests] as a required dependency forces all users to install it unnecessarily. This bloats the core install footprint for an unused feature.
Move it to [project.optional-dependencies]:
Suggested change to pyproject.toml
[project.optional-dependencies]
telemetry = [
"logfire[requests]>=2.11,<3.0",
]Then add proper error handling in src/garth/telemetry.py at the import site (line 167) so users get a clear message if they try to enable telemetry without the extra:
try:
import logfire
except ImportError:
raise ImportError(
"logfire is required for telemetry. Install with: pip install garth[telemetry]"
)Update documentation to mention the telemetry extra.
🤖 Prompt for AI Agents
In @pyproject.toml at line 12, Move the required "logfire[requests]>=2.11,<3.0"
entry out of the main dependencies and into [project.optional-dependencies]
under a new "telemetry" extra in pyproject.toml so users only install it when
they opt-in; then in src/garth/telemetry.py (at the import site around the
current import on line ~167) wrap the logfire import in a try/except ImportError
and raise a clear message instructing users to install the extra (e.g. "pip
install garth[telemetry]"); finally update the README/docs to document the
telemetry extra and how to enable telemetry via configure(enabled=True).
| # Try to sanitize JSON | ||
| try: | ||
| data = json.loads(text) | ||
| if isinstance(data, dict): | ||
| for fld in JSON_SENSITIVE_FIELDS: | ||
| if fld in data: | ||
| data[fld] = REDACTED | ||
| return json.dumps(data) | ||
| except (json.JSONDecodeError, TypeError): | ||
| pass | ||
|
|
||
| return text |
There was a problem hiding this comment.
Nested JSON fields are not sanitized.
The JSON sanitization only checks top-level keys. If a response contains nested sensitive data like {"response": {"access_token": "xxx"}}, it will not be redacted.
Consider a recursive approach or document this limitation if Garmin's APIs only use flat structures.
🔒 Proposed recursive sanitization
+def _sanitize_dict(data: dict) -> dict:
+ """Recursively sanitize sensitive fields in a dict."""
+ for key, value in data.items():
+ if key in JSON_SENSITIVE_FIELDS:
+ data[key] = REDACTED
+ elif isinstance(value, dict):
+ _sanitize_dict(value)
+ elif isinstance(value, list):
+ for item in value:
+ if isinstance(item, dict):
+ _sanitize_dict(item)
+ return data
+
+
def sanitize(text: str) -> str:
"""Sanitize sensitive data from request/response text."""
# Sanitize query params (key=value&...)
for key in QUERY_PARAM_PATTERNS:
text = re.sub(
key + r"=[^&\s]*", f"{key}={REDACTED}", text, flags=re.IGNORECASE
)
# Try to sanitize JSON
try:
data = json.loads(text)
if isinstance(data, dict):
- for fld in JSON_SENSITIVE_FIELDS:
- if fld in data:
- data[fld] = REDACTED
+ _sanitize_dict(data)
return json.dumps(data)
except (json.JSONDecodeError, TypeError):
pass
return text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Try to sanitize JSON | |
| try: | |
| data = json.loads(text) | |
| if isinstance(data, dict): | |
| for fld in JSON_SENSITIVE_FIELDS: | |
| if fld in data: | |
| data[fld] = REDACTED | |
| return json.dumps(data) | |
| except (json.JSONDecodeError, TypeError): | |
| pass | |
| return text | |
| def _sanitize_dict(data: dict) -> dict: | |
| """Recursively sanitize sensitive fields in a dict.""" | |
| for key, value in data.items(): | |
| if key in JSON_SENSITIVE_FIELDS: | |
| data[key] = REDACTED | |
| elif isinstance(value, dict): | |
| _sanitize_dict(value) | |
| elif isinstance(value, list): | |
| for item in value: | |
| if isinstance(item, dict): | |
| _sanitize_dict(item) | |
| return data | |
| def sanitize(text: str) -> str: | |
| """Sanitize sensitive data from request/response text.""" | |
| # Sanitize query params (key=value&...) | |
| for key in QUERY_PARAM_PATTERNS: | |
| text = re.sub( | |
| key + r"=[^&\s]*", f"{key}={REDACTED}", text, flags=re.IGNORECASE | |
| ) | |
| # Try to sanitize JSON | |
| try: | |
| data = json.loads(text) | |
| if isinstance(data, dict): | |
| _sanitize_dict(data) | |
| return json.dumps(data) | |
| except (json.JSONDecodeError, TypeError): | |
| pass | |
| return text |
🤖 Prompt for AI Agents
In @src/garth/telemetry.py around lines 50 - 61, The current sanitization only
replaces top-level keys; update the sanitizer (the function that calls
json.loads and iterates JSON_SENSITIVE_FIELDS) to recursively walk parsed JSON
structures: if the parsed value is a dict, iterate its items and if a key is in
JSON_SENSITIVE_FIELDS replace its value with REDACTED, otherwise recurse into
the value; if the value is a list, recurse into each element; after recursion
return json.dumps of the sanitized structure. Use the existing
JSON_SENSITIVE_FIELDS and REDACTED symbols and ensure you still catch
json.JSONDecodeError and TypeError and return the original text when parsing
fails.
| # Check env var overrides | ||
| env_enabled = os.getenv("GARTH_TELEMETRY", "").lower() | ||
| if env_enabled == "true": | ||
| self.enabled = True | ||
| elif env_enabled == "false": | ||
| self.enabled = False | ||
|
|
||
| env_service_name = os.getenv("GARTH_TELEMETRY_SERVICE_NAME") | ||
| if env_service_name: | ||
| self.service_name = env_service_name | ||
| if os.getenv("LOGFIRE_SEND_TO_LOGFIRE", "").lower() == "false": | ||
| self.send_to_logfire = False |
There was a problem hiding this comment.
Environment variables override programmatic settings.
The current logic applies env var overrides after programmatic settings, meaning configure(enabled=False) can be overridden by GARTH_TELEMETRY=true. This behavior may surprise users who expect programmatic settings to take precedence.
Consider either documenting this clearly or reversing the precedence (apply env defaults first, then allow programmatic overrides).
1 participant
Summary
Configuration
Enable via environment variables:
Or via code:
Use alternative OTLP backend:
Test plan
Closes #161
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.