Integrate Sentry into HTTP handling #112
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReworks startup and HTTP wiring to compose a final handler with environment-aware CORS and optional Sentry wrapping, expands Sentry initialization/options and exposure on App, adds Sentry-backed API error capture, and wires recover/flush into CLI and seeder startups and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server
participant Mux as Mux/Router
participant CORS as CORS Wrapper (non-prod)
participant SentryH as Sentry Handler (optional)
Client->>Server: HTTP Request
Server->>Mux: GetMux()
alt mux is nil
Server-->>Client: NotFound / no handler
else mux present
alt non-production
Server->>CORS: wrap(mux)
CORS->>SentryH: pass-through (if configured)
SentryH->>Mux: ServeHTTP
else production
Server->>SentryH: wrap(mux) (if configured)
SentryH->>Mux: ServeHTTP
end
Mux-->>Client: HTTP Response
end
sequenceDiagram
autonumber
actor Client
participant API as API Handler
participant Sentry as Sentry Hub/Scope
Client->>API: Request
API->>API: process request
alt Error occurs
API->>Sentry: captureApiError(method, route, status, err)
Note right of Sentry: tags: http.method, http.status_code, http.route<br/>level: warning (<500) / error (>=500)
API-->>Client: JSON error response
else Success
API-->>Client: JSON success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 |
Summary of ChangesHello @gocanto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive Sentry integration into the application's HTTP request handling. The primary goal is to enhance error monitoring by automatically capturing API errors, providing detailed context, and enabling distributed tracing. This ensures that issues are more easily identified and debugged across different environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively integrates Sentry for error tracking in the HTTP handling layer. The Sentry client is well-configured with environment metadata and tracing options, and the middleware is correctly applied to the server handler. The new captureApiError function is a great addition for sending detailed error reports to Sentry. I've found a critical issue that will prevent compilation, and have also included a couple of suggestions to further improve the Sentry integration for more effective error tracking and to enhance code clarity.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
main.go(1 hunks)metal/kernel/factory.go(2 hunks)metal/kernel/helpers.go(2 hunks)metal/kernel/kernel_test.go(3 hunks)pkg/http/handler.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
metal/kernel/kernel_test.go (3)
metal/router/static.go (2)
WebsiteRoutes(14-20)NewWebsiteRoutes(22-30)metal/router/router.go (1)
WebsiteRoutes(16-23)metal/kernel/app.go (2)
router(17-24)a(81-100)
pkg/http/handler.go (2)
pkg/http/schema.go (2)
ApiHandler(25-25)ApiError(11-15)pkg/http/handler_test.go (1)
TestMakeApiHandler(10-35)
metal/kernel/factory.go (3)
pkg/portal/sentry.go (1)
Sentry(8-12)metal/env/env.go (1)
Environment(9-17)metal/env/sentry.go (1)
DSN(3-6)
main.go (1)
pkg/portal/sentry.go (1)
Handler(8-12)
metal/kernel/helpers.go (3)
metal/kernel/app.go (3)
App(17-24)a(81-100)MakeApp(26-42)pkg/portal/sentry.go (1)
Sentry(8-12)metal/env/env.go (1)
App(9-17)
🔇 Additional comments (4)
metal/kernel/helpers.go (1)
48-50: Sentry accessor rounds out the kernel APIThe accessor cleanly exposes the Sentry handle so upstream wiring can stay decoupled. Looks good to me.
metal/kernel/factory.go (1)
16-47: Environment-aware Sentry setup looks solidApp metadata, tracing, and delivery knobs are all wired in coherently. Nicely done.
pkg/http/handler.go (1)
35-68: Great addition for scoped Sentry captureThe scoped capture covers the critical HTTP context and gracefully falls back to the global hub. This will make diagnosing API errors much easier.
metal/kernel/kernel_test.go (1)
77-176: Tests now mirror the richer environmentThanks for keeping the fixtures in sync with the new env requirements and router wiring. The additional coverage is appreciated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
metal/kernel/factory.go (3)
21-21: Avoid forcing an unversioned Release; rely on SENTRY_RELEASE or include a versionHardcoding
Release: env.App.Namedegrades grouping. Either remove it (letSENTRY_RELEASEenv var drive it) or include a proper version (e.g., app@1.2.3 or git SHA).Apply this diff to remove the forced release so Sentry can use
SENTRY_RELEASE:- Release: env.App.Name,
23-24: Make trace sampling configurable; 100% in production is costly
EnableTracing: truewithTracesSampleRate: 1.0will sample every request. Consider making the sample rate env‑configurable and defaulting lower in prod.
17-25: Add a BeforeSend scrubber to redact sensitive headersYou call
scope.SetRequest(r)in request handlers; proactively redactAuthorizationandCookieto reduce PII risk.func MakeSentry(env *env.Environment) *portal.Sentry { cOptions := sentry.ClientOptions{ Dsn: env.Sentry.DSN, Debug: env.App.IsLocal(), Environment: env.App.Type, - Release: env.App.Name, AttachStacktrace: true, EnableTracing: true, TracesSampleRate: 1.0, + BeforeSend: func(event *sentry.Event, hint *sentry.EventHint) *sentry.Event { + if event.Request != nil && event.Request.Headers != nil { + if _, ok := event.Request.Headers["Authorization"]; ok { + event.Request.Headers["Authorization"] = "[Filtered]" + } + if _, ok := event.Request.Headers["Cookie"]; ok { + event.Request.Headers["Cookie"] = "[Filtered]" + } + } + return event + }, }pkg/http/handler.go (2)
35-69: Prefer not capturing 4xx by default to reduce noiseMost teams don’t send 4xx to Sentry. Consider skipping capture for
Status < 500.func captureApiError(r *baseHttp.Request, apiErr *ApiError) { if apiErr == nil { return } - level := sentry.LevelWarning - if apiErr.Status >= baseHttp.StatusInternalServerError { - level = sentry.LevelError - } + if apiErr.Status < baseHttp.StatusInternalServerError { + return + } + level := sentry.LevelError
12-31: Expose Sentry event ID to clients for support correlationReturn the event ID from
captureApiErrorand setX-Sentry-Event-Idin the response.func MakeApiHandler(fn ApiHandler) baseHttp.HandlerFunc { return func(w baseHttp.ResponseWriter, r *baseHttp.Request) { if err := fn(w, r); err != nil { slog.Error("API Error", "message", err.Message, "status", err.Status) - captureApiError(r, err) + if eid := captureApiError(r, err); eid != "" { + w.Header().Set("X-Sentry-Event-Id", eid) + } w.Header().Set("Content-Type", "application/json") w.WriteHeader(err.Status)-func captureApiError(r *baseHttp.Request, apiErr *ApiError) { +func captureApiError(r *baseHttp.Request, apiErr *ApiError) string { if apiErr == nil { - return + return "" } level := sentry.LevelWarning if apiErr.Status >= baseHttp.StatusInternalServerError { level = sentry.LevelError } + var eventID sentry.EventID notify := func(hub *sentry.Hub) { hub.WithScope(func(scope *sentry.Scope) { scope.SetLevel(level) scope.SetTag("http.method", r.Method) scope.SetTag("http.status_code", strconv.Itoa(apiErr.Status)) scope.SetTag("http.route", r.URL.Path) scope.SetRequest(r) scope.SetExtra("api_error_status_text", baseHttp.StatusText(apiErr.Status)) if apiErr.Data != nil { scope.SetExtra("api_error_data", apiErr.Data) } - hub.CaptureException(apiErr) + eventID = hub.CaptureException(apiErr) }) } if hub := sentry.GetHubFromContext(r.Context()); hub != nil { notify(hub) - return + return string(eventID) } notify(sentry.CurrentHub()) + return string(eventID) }Also applies to: 35-69
main.go (1)
90-92: Avoid shadowing the sentry package name
sentby := app.GetSentry()(orsentryHub) reads clearer and prevents accidental package shadowing if used later.- if sentry := app.GetSentry(); sentry != nil && sentry.Handler != nil { - handler = sentry.Handler.Handle(handler) + if sh := app.GetSentry(); sh != nil && sh.Handler != nil { + handler = sh.Handler.Handle(handler) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
database/seeder/main.go(3 hunks)main.go(2 hunks)metal/cli/main.go(2 hunks)metal/kernel/factory.go(2 hunks)metal/kernel/helpers.go(3 hunks)metal/kernel/kernel_test.go(4 hunks)pkg/http/handler.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- metal/kernel/helpers.go
- metal/kernel/kernel_test.go
🧰 Additional context used
🧬 Code graph analysis (5)
metal/cli/main.go (4)
pkg/portal/sentry.go (1)
Sentry(8-12)metal/kernel/factory.go (1)
MakeSentry(16-44)metal/kernel/helpers.go (1)
RecoverWithSentry(41-43)metal/env/sentry.go (1)
DSN(3-6)
pkg/http/handler.go (1)
pkg/http/schema.go (3)
ApiHandler(25-25)ApiError(11-15)e(17-23)
main.go (1)
pkg/portal/sentry.go (1)
Handler(8-12)
database/seeder/main.go (4)
pkg/portal/sentry.go (1)
Sentry(8-12)metal/kernel/factory.go (1)
MakeSentry(16-44)metal/kernel/helpers.go (1)
RecoverWithSentry(41-43)metal/env/sentry.go (1)
DSN(3-6)
metal/kernel/factory.go (4)
pkg/portal/sentry.go (1)
Sentry(8-12)metal/kernel/app.go (1)
App(17-24)metal/env/env.go (1)
Environment(9-17)metal/env/sentry.go (1)
DSN(3-6)
🔇 Additional comments (6)
metal/cli/main.go (1)
28-29: Sentry lifecycle wiring looks correct; confirm recover helper is nil‑safeInit + defer patterns are fine. Please confirm
kernel.RecoverWithSentrytolerates a nil hub and doesn’t callos.Exit, sosentry.Flushcan run.Also applies to: 34-36
database/seeder/main.go (1)
23-24: Seeder Sentry integration LGTM; confirm recover helper behaviorInitialization and defers look good. Please verify
kernel.RecoverWithSentryhandles nil hubs and doesn’t bypass other defers (e.g., viaos.Exit).Also applies to: 32-36
metal/kernel/factory.go (1)
31-35: HTTP integration options look good
Repanic,WaitForDeliveryin prod, andTimeoutare sane defaults.pkg/http/handler.go (1)
12-19: Good addition: capture API errors before respondingCapturing via Sentry prior to encoding the response is the right spot.
main.go (2)
31-35: Server panic recovery + flush ordering LGTMStartup defers cover panic recovery and flushing. Good.
61-61: Fix CORS origin: add scheme to match browser Origin headerPassing
host:portwon’t matchOrigin: http://host:port. Prepend the scheme.- localhost := app.GetEnv().Network.GetHostURL() + localhost := fmt.Sprintf("http://%s", app.GetEnv().Network.GetHostURL()) c := cors.New(cors.Options{ - AllowedOrigins: []string{localhost, "http://localhost:5173"}, + AllowedOrigins: []string{localhost, "http://localhost:5173"},Also applies to: 79-81
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68d4e11962788333946c514b477f7ef3
Summary by CodeRabbit
New Features
Refactor
Tests