-
Notifications
You must be signed in to change notification settings - Fork 18
feat(monitoring): added Sentry support for monitoring #341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideAdds optional Sentry monitoring integration gated by configuration, wires it into the FastAPI app startup, introduces a Sentry DSN setting, and declares the sentry-sdk dependency, along with a minor version bump. Sequence diagram for FastAPI app startup with optional Sentry initializationsequenceDiagram
participant FastAPIApp
participant main_module
participant Settings
participant SentrySDK
FastAPIApp->>main_module: import app.main
main_module->>Settings: load settings
alt sentry_dsn is non empty
main_module->>SentrySDK: init(dsn, send_default_pii, enable_logs, traces_sample_rate, profile_session_sample_rate, profile_lifecycle)
SentrySDK-->>main_module: Sentry client initialized
else sentry_dsn is empty
main_module-->>FastAPIApp: Sentry not initialized
end
main_module-->>FastAPIApp: FastAPI instance with lifespan
Updated class diagram for Settings with Sentry configurationclassDiagram
class BaseSettings
class Settings {
+string sentry_dsn
}
BaseSettings <|-- Settings
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- Consider making Sentry tuning parameters (PII collection, traces_sample_rate, profile_session_sample_rate, profile_lifecycle) configurable via Settings instead of hard-coding them in main.py so they can be adjusted per environment without code changes.
- Since you are depending on sentry-sdk[fastapi], you may want to explicitly use FastAPI and logging integrations (e.g., FastApiIntegration, LoggingIntegration) in sentry_sdk.init to ensure framework-specific context and logs are captured correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making Sentry tuning parameters (PII collection, traces_sample_rate, profile_session_sample_rate, profile_lifecycle) configurable via Settings instead of hard-coding them in main.py so they can be adjusted per environment without code changes.
- Since you are depending on sentry-sdk[fastapi], you may want to explicitly use FastAPI and logging integrations (e.g., FastApiIntegration, LoggingIntegration) in sentry_sdk.init to ensure framework-specific context and logs are captured correctly.
## Individual Comments
### Comment 1
<location> `app/main.py:36-45` </location>
<code_context>
+if settings.sentry_dsn:
+ import sentry_sdk
+
+ sentry_sdk.init(
+ dsn=settings.sentry_dsn,
+ # Add data like request headers and IP for users,
+ # see https://docs.sentry.io/platforms/python/data-management/data-collected/ for more info
+ send_default_pii=True,
+ # Enable sending logs to Sentry
+ enable_logs=True,
+ # Set traces_sample_rate to 1.0 to capture 100%
+ # of transactions for tracing.
+ traces_sample_rate=1.0,
+ # Set profile_session_sample_rate to 1.0 to profile 100%
+ # of profile sessions.
+ profile_session_sample_rate=1.0,
+ # Set profile_lifecycle to "trace" to automatically
+ # run the profiler on when there is an active transaction
+ profile_lifecycle="trace",
+ )
+
</code_context>
<issue_to_address>
**suggestion:** Leverage the FastAPI-specific integration from `sentry-sdk[fastapi]` to get richer context and better error reporting.
Given we already depend on the `fastapi` extra, consider initializing Sentry with the FastAPI integration (e.g. `integrations=[sentry_sdk.integrations.fastapi.FastApiIntegration()]`). This will attach richer request/response context, better transaction naming, and improved handling of background tasks compared to using the default `init` alone.
Suggested implementation:
```python
from .players import router as players
from .roles import router as roles
if settings.sentry_dsn:
import sentry_sdk
from sentry_sdk.integrations.fastapi import FastApiIntegration
sentry_sdk.init(
dsn=settings.sentry_dsn,
integrations=[FastApiIntegration()],
# Add data like request headers and IP for users,
# see https://docs.sentry.io/platforms/python/data-management/data-collected/ for more info
send_default_pii=True,
# Enable sending logs to Sentry
enable_logs=True,
```
If there are further keyword arguments to `sentry_sdk.init` after `enable_logs=True,` in the real file (e.g. `traces_sample_rate`, `profile_session_sample_rate`, etc.), they will remain unchanged after this edit and will continue to work as before. No additional code changes should be needed as long as `settings` is already imported and configured in this module.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.



Summary by Sourcery
Add optional Sentry-based monitoring and bump project version.
New Features:
Build: