fix: remove sentry hard dependency #494
Conversation
WalkthroughModified Sentry initialization in production setup to conditionally init with explicit integrations (FastApiIntegration, LoggingIntegration, StdlibIntegration) while disabling defaults. Added try-except error handling wrapper for non-fatal initialization failures. Changes
Sequence Diagram(s)sequenceDiagram
participant App as app/main.py
participant Sentry
participant Logger
alt Old Flow
App->>Sentry: sentry_sdk.init()
Note over Sentry: Default integrations auto-loaded
end
alt New Flow
App->>App: Check if production
alt Production
App->>App: try block
App->>Sentry: sentry_sdk.init(integrations=[...])
Note over Sentry: Defaults disabled<br/>FastApi, Logging, Stdlib only
Sentry-->>App: Success
else Init fails
App->>Logger: Log error (non-fatal)
Note over Logger: Continue startup
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/main.py (1)
5-5: Critical: Top-level import defeats the "remove hard dependency" objective.The
import sentry_sdkat module level will cause the entire application to crash if Sentry is not installed, even beforesetup_sentryis called. This contradicts the PR's goal of making Sentry optional.Apply this diff to move the import inside the conditional block:
-import sentry_sdk from dotenv import load_dotenv from fastapi import FastAPI from fastapi.middleware.cors import CORSMiddlewareThen update the
setup_sentrymethod to import sentry_sdk inside the try block:def setup_sentry(self): if os.getenv("ENV") == "production": try: + import sentry_sdk # Explicitly configure integrations to avoid auto-enabling Strawberry # which causes crashes when Strawberry is not installed from sentry_sdk.integrations.fastapi import FastApiIntegration
🧹 Nitpick comments (1)
app/main.py (1)
72-72: Consider reducing profiles_sample_rate for production.The
profiles_sample_rate=1.0(100%) may generate significant overhead and cost in production, especially as traffic scales. Consider aligning it withtraces_sample_rateor using a lower value (e.g., 0.1 or 0.25).sentry_sdk.init( dsn=os.getenv("SENTRY_DSN"), traces_sample_rate=0.25, - profiles_sample_rate=1.0, + profiles_sample_rate=0.25, default_integrations=False, integrations=[
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/main.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
app/main.py
80-80: Do not catch blind exception: Exception
(BLE001)
| try: | ||
| # Explicitly configure integrations to avoid auto-enabling Strawberry | ||
| # which causes crashes when Strawberry is not installed | ||
| from sentry_sdk.integrations.fastapi import FastApiIntegration | ||
| from sentry_sdk.integrations.logging import LoggingIntegration | ||
| from sentry_sdk.integrations.stdlib import StdlibIntegration | ||
|
|
||
| sentry_sdk.init( | ||
| dsn=os.getenv("SENTRY_DSN"), | ||
| traces_sample_rate=0.25, | ||
| profiles_sample_rate=1.0, | ||
| default_integrations=False, | ||
| integrations=[ | ||
| FastApiIntegration(), | ||
| LoggingIntegration(), | ||
| StdlibIntegration(), | ||
| ], | ||
| ) | ||
| except Exception as e: | ||
| logging.warning(f"Sentry initialization failed (non-fatal): {e}") |
There was a problem hiding this comment.
🧩 Analysis chain
Good approach to make Sentry optional with explicit integrations.
The explicit integration configuration correctly prevents auto-enabling of optional integrations like Strawberry. The try-except wrapper makes initialization non-fatal as intended.
Regarding the static analysis hint about catching Exception: In this context, it's acceptable because:
- Sentry is an optional observability feature, not core functionality
- The exception is logged with context (line 81), not silently swallowed
- Making it non-fatal allows the application to continue without Sentry
However, ensure that after fixing the top-level import issue (line 5), the application starts successfully both with and without sentry-sdk installed:
🏁 Script executed:
#!/bin/bash
# Verify Sentry is truly optional by checking for any remaining hard dependencies
# Check for any other sentry_sdk imports at module level
rg -n "^import sentry_sdk|^from sentry_sdk" --type=py
# Check if sentry_sdk is in required dependencies (should be optional)
fd -e txt -e toml -e cfg --exec grep -l "sentry" {} \;Length of output: 50
Let me check the exact state of the import at line 5 and the dependency status:
🏁 Script executed:
#!/bin/bash
# Get context around line 5 of app/main.py
head -n 15 app/main.py | tail -n +1
# Get requirements.txt content
cat ./requirements.txtLength of output: 2170
🏁 Script executed:
#!/bin/bash
# View lines 1-20 of app/main.py to see the import context
sed -n '1,20p' app/main.py
# Check if sentry-sdk is optional in requirements.txt
grep -i "sentry" ./requirements.txtLength of output: 828
Line 5 unconditional import prevents Sentry from being optional—fix required before trying approach is meaningful.
The module-level import sentry_sdk at line 5 will crash the app with an ImportError if sentry-sdk is not installed, bypassing the try-except block at lines 62–81. Since sentry-sdk[fastapi]==2.20.0 is currently a required dependency in requirements.txt, the initialization wrapping alone does not achieve the goal of optional Sentry.
To make Sentry truly optional:
-
Wrap the import at line 5 in a try-except:
try: import sentry_sdk except ImportError: sentry_sdk = None
-
Add a guard before calling
sentry_sdk.init()at line 62:if sentry_sdk is not None: try: # existing init code
-
Mark sentry-sdk as an optional dependency in requirements.txt (if using extras) or document it as an optional add-on.
🧰 Tools
🪛 Ruff (0.14.5)
80-80: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In app/main.py around lines 62 to 81 (and addressing the module-level import at
line 5), the module-level "import sentry_sdk" will raise ImportError when
sentry-sdk isn't installed and bypasses the runtime try/except around init; wrap
the top-level import in a try/except that sets sentry_sdk = None on ImportError,
then guard the block at lines 62-81 so you only call sentry_sdk.init(...) when
sentry_sdk is not None (i.e., skip initialization and keep the existing except
logging otherwise); finally, mark or document sentry-sdk as optional in
requirements.txt or extras so the dependency is not required by default.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.