Skip to content

Feat/sentry alertmanager emailer#186

Closed
andrewwu13 wants to merge 9 commits intomainfrom
feat/sentry-alertmanager-emailer
Closed

Feat/sentry alertmanager emailer#186
andrewwu13 wants to merge 9 commits intomainfrom
feat/sentry-alertmanager-emailer

Conversation

@andrewwu13
Copy link
Copy Markdown
Member

@andrewwu13 andrewwu13 commented Jan 28, 2026

User description

What is this issue for and how does it solve it

integrate sentry logging into our log handler. all logs should also be sent to the sentry app, can configure this later.

Link to the Github Issue

Addresses #167


PR Type

Enhancement


Description

  • Integrate native Sentry logging for centralized error monitoring

  • Add SentryLoggerWrapper to dual-log to standard logging and Sentry

  • Implement Prometheus metrics and Grafana monitoring stack

  • Add request duration tracking per endpoint with middleware

  • Configure Sentry SDK with FastAPI and logging integrations

  • Add monitoring services (Prometheus, Grafana) to docker-compose


Diagram Walkthrough

flowchart LR
  A["Application Logs"] --> B["SentryLoggerWrapper"]
  B --> C["Standard Logger"]
  B --> D["Sentry SDK"]
  C --> E["Console/File Output"]
  D --> F["Sentry Cloud"]
  G["FastAPI App"] --> H["Metrics Middleware"]
  H --> I["Prometheus"]
  I --> J["Grafana Dashboard"]
Loading

File Walkthrough

Relevant files
Enhancement
2 files
logging_utils.py
Add SentryLoggerWrapper for dual logging to Sentry             
+105/-18
main.py
Initialize Sentry SDK and add Prometheus metrics                 
+49/-0   
Formatting
3 files
google_drive.py
Clean up import statement formatting                                         
+0/-1     
image_processing.py
Reorder imports for consistency                                                   
+1/-1     
user_service.py
Reorder imports for consistency                                                   
+1/-1     
Configuration changes
3 files
docker-compose.yml
Add Prometheus and Grafana monitoring services                     
+39/-0   
prometheus.yml
Configure Prometheus scrape targets                                           
+13/-0   
datasources.yml
Configure Grafana Prometheus data source                                 
+7/-0     
Dependencies
1 files
pyproject.toml
Add prometheus-fastapi-instrumentator dependency                 
+2/-1     

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jan 28, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🔴
Hardcoded Sentry DSN

Description: A hardcoded Sentry DSN embeds a production-like external monitoring endpoint in source
code, which can leak project identifiers and enable unintended data exfiltration to Sentry
if deployed as-is.
main.py [43-52]

Referred Code
sentry_sdk.init(
    dsn="https://c217aed2d06bdc4504801adf99840b54@o4510432441860096.ingest.us.sentry.io/4510784901677056",
    integrations=[
        FastApiIntegration(),
        # Disable legacy breadcrumb/event behavior - we use native Sentry logs via sentry_sdk.logger
        LoggingIntegration(event_level=None, level=None),
    ],
    traces_sample_rate=1.0,
    enable_logs=True,  # Enable Sentry's native structured logs
)
Debug endpoint exposure

Description: The publicly accessible /sentry-debug endpoint intentionally raises an exception (division
by zero), enabling trivial remote error generation and potential denial-of-service/log
flooding in deployed environments.
main.py [129-133]

Referred Code
@app.get("/sentry-debug")
async def trigger_error():
    division_by_zero = 1 / 0
    return division_by_zero
Ticket Compliance
🟡
🎫 #167
🟢 Add monitoring services to docker-compose.yml under a separate profile so docker compose
--profile monitoring up runs monitoring containers + the website, while docker compose up
runs only the website.
🔴 Prefer a free/open-source, self-hosted stack; suggested: OpenTelemetry + Loki + Prometheus
+ Tempo + Grafana.
Add self-hosted monitoring so performance, logs, and tracebacks can be viewed on demand.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Invalid logging API: The new code calls non-existent logger/handler methods (set_level, add_handler) which will
raise runtime errors and break logging initialization/notification paths.

Referred Code
    def set_level(self, level: int) -> None:
        self._std_logger.set_level(level)

    def add_handler(self, handler: logging.Handler) -> None:
        self._std_logger.add_handler(handler)


def setup_logger(name: str) -> SentryLoggerWrapper:
    """Set up a logger with the specified name.

    Returns a wrapper that logs to both standard Python logging (console/file)
    and Sentry structured logs for centralized monitoring.

    Args:
        name (str): The name of the logger.

    Returns:
        SentryLoggerWrapper: A logger that outputs to console, file, and Sentry.
    """
    std_logger = logging.getLogger(name)
    std_logger.set_level(logging.INFO)


 ... (clipped 97 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Debug crash endpoint: The newly added /sentry-debug endpoint intentionally triggers a ZeroDivisionError, which
risks exposing stack traces/internal details to end users if deployed without strict
environment gating.

Referred Code
@app.get("/sentry-debug")
async def trigger_error():
    division_by_zero = 1 / 0
    return division_by_zero

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Hardcoded Sentry DSN: The PR hardcodes the Sentry DSN in source code instead of sourcing it from secure
configuration, increasing the risk of credential/tenant leakage via the repository and
logs/config dumps.

Referred Code
sentry_sdk.init(
    dsn="https://c217aed2d06bdc4504801adf99840b54@o4510432441860096.ingest.us.sentry.io/4510784901677056",
    integrations=[
        FastApiIntegration(),
        # Disable legacy breadcrumb/event behavior - we use native Sentry logs via sentry_sdk.logger
        LoggingIntegration(event_level=None, level=None),
    ],
    traces_sample_rate=1.0,
    enable_logs=True,  # Enable Sentry's native structured logs
)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jan 28, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove merge conflict markers

Remove the Git merge conflict markers from prometheus.yml to ensure the file is
valid YAML and can be parsed correctly by Prometheus.

monitoring/prometheus/prometheus.yml [3-6]

-<<<<<<< HEAD
-=======
->>>>>>> bb03367 (removed loki + tempo traces, fixed prometheus issues)
+global:
+  scrape_interval: 5s
 
+scrape_configs:
+  - job_name: 'prometheus'
+    static_configs:
+      - targets: ['prometheus:9090']
+  - job_name: 'app'
+    static_configs:
+      - targets: ['app:9000']
+
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies Git merge conflict markers in prometheus.yml which would cause the Prometheus service to fail on startup, making this a critical fix.

High
Use correct logging API methods
Suggestion Impact:Updated std_logger calls from add_handler(...) to addHandler(...) for console, file, and email handlers (but did not change any set_level usages in this diff).

code diff:

-        std_logger.add_handler(console_handler)
+        std_logger.addHandler(console_handler)
 
         # File handler for persistent logging
         file_handler = _setup_file_handler()
         if file_handler:
-            std_logger.add_handler(file_handler)
+            std_logger.addHandler(file_handler)
 
         # Email handler for errors (if configured)
         email_handler = _setup_email_handler()
         if email_handler:
-            std_logger.add_handler(email_handler)
+            std_logger.addHandler(email_handler)

In src/core/logging_utils.py, replace the incorrect method calls set_level and
add_handler with the correct standard logging methods setLevel and addHandler.

src/core/logging_utils.py [119-215]

-std_logger.set_level(logging.INFO)
+std_logger.setLevel(logging.INFO)
 ...
-std_logger.add_handler(console_handler)
+std_logger.addHandler(console_handler)
 ...
-email_handler.set_level(logging.ERROR)
+email_handler.setLevel(logging.ERROR)

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies the use of incorrect method names (set_level, add_handler) which would cause a runtime AttributeError and prevent the logging system from being configured.

High
Correct Prometheus scrape port for application

Correct the Prometheus scrape target for the 'app' job from port 9000 to 8000 to
match the application's actual running port.

monitoring/prometheus/prometheus.yml [11-13]

 - job_name: 'app'
   static_configs:
-    - targets: ['app:9000']
+    - targets: ['app:8000']
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical port mismatch between the application's runtime configuration and the Prometheus scrape configuration, which would prevent application metric collection.

High
Capture exception stack traces in Sentry

Modify the SentryLoggerWrapper.exception method to use
sentry_sdk.capture_exception() to ensure full exception details and stack traces
are sent to Sentry.

src/core/logging_utils.py [89-92]

 def exception(self, msg: str, *args, **kwargs) -> None:
     """Log exception with traceback to console/file and Sentry."""
     self._std_logger.exception(msg, *args, **kwargs)
     self._log_to_sentry("error", msg, *args, **kwargs)
+    try:
+        from sentry_sdk import capture_exception
+        # logger.exception is expected to be called in an except block,
+        # so capture_exception will find the exception info.
+        capture_exception()
+    except ImportError:
+        pass # Sentry not installed or available
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the custom Sentry logging wrapper fails to capture stack traces for exceptions and provides the correct fix using sentry_sdk.capture_exception().

Medium
General
Validate presence of smtp_port

Add smtp_port to the all() check in _setup_email_handler to ensure its presence
is validated before it is used, preventing a potential runtime error.

src/core/logging_utils.py [198-199]

-if not all([smtp_server, smtp_username, smtp_password, from_email, to_emails]):
+if not all([smtp_server, smtp_port, smtp_username, smtp_password, from_email, to_emails]):
     return None

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that smtp_port is missing from the validation check, which would lead to a TypeError when int() is called on None if the port is not configured.

Medium
Remove redundant custom metrics middleware

Remove the custom metrics_middleware as its functionality for tracking request
latency is already provided by the prometheus-fastapi-instrumentator library,
which is also configured.

src/main.py [83-96]

-@app.middleware("http")
-async def metrics_middleware(request: Request, call_next):
-    start_time = datetime.now()
-    response = await call_next(request)
-    process_time = (datetime.now() - start_time).total_seconds()
+# This middleware should be removed.
+# The Instrumentator().instrument(app) call already provides request latency metrics.
 
-    endpoint = request.scope.get("path", "unknown")
-    if request.scope.get("route"):
-        endpoint = request.scope["route"].path
-
-    REQUEST_LATENCY.labels(
-        method=request.method, endpoint=endpoint, status_code=response.status_code
-    ).observe(process_time)
-    return response
-
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the custom metrics_middleware is redundant because prometheus-fastapi-instrumentator already provides the same functionality, improving code by removing unnecessary duplication.

Medium
  • Update

Comment thread .github/workflows/supabase-activity.yml Fixed
@andrewwu13 andrewwu13 force-pushed the feat/sentry-alertmanager-emailer branch from 18a1b84 to 8e5b2cd Compare March 11, 2026 23:39
Copy link
Copy Markdown
Member

@rajpandya737 rajpandya737 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we moving to sentry, make a new PR without all the prometheus and grafana stuff

@andrewwu13 andrewwu13 closed this Apr 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants