Conversation
WalkthroughLogfire was moved from required to an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 #191 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 68 68
Lines 3489 3508 +19
=========================================
+ Hits 3489 3508 +19
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:
|
matin
force-pushed
the
issue-190-optional-telemetry
branch
from
5e6cf4f to
57a0ee2
Compare
Move logfire from required dependencies to optional extras to avoid installing OpenTelemetry packages for users who don't need telemetry. Install with: pip install garth[telemetry] Fixes #190 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
matin
force-pushed
the
issue-190-optional-telemetry
branch
from
57a0ee2 to
d78e12d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_telemetry.py`:
- Around line 300-309: The tests mock the logfire module but never set the
feature flag, so _configure_logfire and _default_callback short-circuit; update
the test blocks (where telemetry_module is imported and mock_logfire is set) to
also set telemetry_module.LOGFIRE_AVAILABLE = True via monkeypatch or direct
assignment so the mocked logfire is actually used — e.g., after
monkeypatch.setattr(telemetry_module, "logfire", mock_logfire) set
telemetry_module.LOGFIRE_AVAILABLE = True (do this in the test blocks around
lines where mock_logfire is created: the blocks covering the imports and mock
setup that include telemetry_module and mock_logfire).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
docs/telemetry.mdpyproject.tomlsrc/garth/telemetry.pysrc/garth/version.pytests/test_telemetry.py
✅ Files skipped from review due to trivial changes (1)
- src/garth/version.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- src/garth/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 (1)
tests/test_telemetry.py (1)
src/garth/telemetry.py (3)
Telemetry(102-234)_default_callback(112-116)_configure_logfire(203-221)
⏰ 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). (2)
- GitHub Check: test 3.12
- GitHub Check: test 3.10
🔇 Additional comments (3)
docs/telemetry.md (2)
5-6: Nice clarity tweak.The capitalization reads well and keeps the emphasis consistent.
8-14: Good addition to document the optional extra.The install instructions make the opt‑in dependency model explicit.
tests/test_telemetry.py (1)
441-469: Good coverage for logfire-absent scenarios.These tests exercise the new guard paths cleanly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import garth.telemetry as telemetry_module | ||
|
|
||
| monkeypatch.setenv("GARTH_TELEMETRY", "true") | ||
| monkeypatch.setenv("LOGFIRE_TOKEN", "test-token") | ||
| monkeypatch.delenv("GARTH_TELEMETRY_SERVICE_NAME", raising=False) | ||
| monkeypatch.delenv("LOGFIRE_SEND_TO_LOGFIRE", raising=False) | ||
|
|
||
| mock_logfire = MagicMock() | ||
| monkeypatch.setitem(sys.modules, "logfire", mock_logfire) | ||
| monkeypatch.setattr(telemetry_module, "logfire", mock_logfire) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the test file at the specified lines
wc -l tests/test_telemetry.pyRepository: matin/garth
Length of output: 82
🏁 Script executed:
# Read the test file sections mentioned in the review
sed -n '300,309p' tests/test_telemetry.pyRepository: matin/garth
Length of output: 441
🏁 Script executed:
# Read the other affected test sections
sed -n '321,328p' tests/test_telemetry.py
sed -n '422,426p' tests/test_telemetry.pyRepository: matin/garth
Length of output: 454
🏁 Script executed:
# Now read the telemetry module to understand LOGFIRE_AVAILABLE and related logic
fd telemetry.py --type fRepository: matin/garth
Length of output: 101
🏁 Script executed:
# Get the telemetry module file
find . -name telemetry.py -type f | grep -v testRepository: matin/garth
Length of output: 79
🏁 Script executed:
# Read the telemetry module to understand LOGFIRE_AVAILABLE logic
wc -l ./src/garth/telemetry.pyRepository: matin/garth
Length of output: 83
🏁 Script executed:
# Get the full telemetry module
cat -n ./src/garth/telemetry.pyRepository: matin/garth
Length of output: 8864
🏁 Script executed:
# Read the test functions around the specified lines to understand what they're testing
sed -n '290,340p' tests/test_telemetry.pyRepository: matin/garth
Length of output: 1578
🏁 Script executed:
# Also read the test around 422-426
sed -n '410,435p' tests/test_telemetry.pyRepository: matin/garth
Length of output: 725
🏁 Script executed:
# Read more of the test_default_callback_calls_logfire test to see what assertions it has
sed -n '422,450p' tests/test_telemetry.pyRepository: matin/garth
Length of output: 841
Set LOGFIRE_AVAILABLE = True when mocking logfire in tests.
These tests mock logfire but don't set LOGFIRE_AVAILABLE = True, causing _configure_logfire (line 205-206) and _default_callback (line 114-115) to short-circuit and never call the mock. Tests at lines 300-309 and 422-426 will fail when asserting logfire was called.
🔧 Suggested fix
mock_logfire = MagicMock()
monkeypatch.setattr(telemetry_module, "logfire", mock_logfire)
+ monkeypatch.setattr(telemetry_module, "LOGFIRE_AVAILABLE", True)Apply this fix to all three test locations: 300-309, 321-328, and 422-426.
🤖 Prompt for AI Agents
In `@tests/test_telemetry.py` around lines 300 - 309, The tests mock the logfire
module but never set the feature flag, so _configure_logfire and
_default_callback short-circuit; update the test blocks (where telemetry_module
is imported and mock_logfire is set) to also set
telemetry_module.LOGFIRE_AVAILABLE = True via monkeypatch or direct assignment
so the mocked logfire is actually used — e.g., after
monkeypatch.setattr(telemetry_module, "logfire", mock_logfire) set
telemetry_module.LOGFIRE_AVAILABLE = True (do this in the test blocks around
lines where mock_logfire is created: the blocks covering the imports and mock
setup that include telemetry_module and mock_logfire).
1 participant
Summary
[telemetry]extraInstall with telemetry
Fixes #190
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.