chore: Improve Sentry context for HTTP API errors #144
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. WalkthroughAdds an Err field and Unwrap to ApiError; response helpers now attach wrapped errors. Introduces ScopeApiError to enrich Sentry scopes with request, account, and API-error metadata and switches handler capture to the most precise error. Tests added for unwrap, request ID/account extraction, enrichment, and error-chain building. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant H as HTTP Handler
participant S as Sentry Hub
participant R as Request/Context
C->>H: HTTP request
H->>R: read headers/context (request_id, account, username, origin, nonce, public_key, client IP)
H->>H: build ApiError (may wrap Err)
H->>H: NewScopeApiError(scope, request, apiErr)
H->>H: ScopeApiError.Enrich() //-- populate scope tags/extras (api_*, http.*, account, request_id, cause_chain)
alt ApiError has wrapped Err
H->>S: CaptureException(ApiError.Err) %% preferred capture
else
H->>S: CaptureException(ApiError)
end
H-->>C: JSON error response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 significantly improves the observability and debuggability of HTTP API errors by enhancing the Sentry integration. It ensures that Sentry captures the root cause of errors and provides a wealth of contextual information about the request, facilitating quicker identification and resolution of issues, especially for unauthorized or bad request failures. The changes also introduce proper error wrapping for 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 significantly improves Sentry error reporting for HTTP API errors. It correctly captures the underlying error cause by implementing error wrapping for ApiError and enriches Sentry events with valuable request context, which will make debugging much easier. The changes are well-structured and accompanied by thorough unit tests. I have a couple of minor suggestions to simplify the code and avoid sending redundant data to Sentry.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/http/handler.go (1)
82-123: Consider adding a doc comment for the enrichment function.The
enrichScopeWithApiErrorfunction performs extensive enrichment and would benefit from a doc comment explaining its purpose and the metadata it extracts.Add a doc comment like:
+// enrichScopeWithApiError populates the Sentry scope with API error details, +// request metadata, and account information to aid debugging. func enrichScopeWithApiError(scope *sentry.Scope, r *baseHttp.Request, apiErr *ApiError) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/http/handler.go(3 hunks)pkg/http/handler_test.go(2 hunks)pkg/http/response.go(3 hunks)pkg/http/schema.go(2 hunks)pkg/http/schema_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/http/schema_test.go (1)
pkg/http/schema.go (1)
ApiError(11-16)
pkg/http/handler_test.go (1)
pkg/portal/consts.go (4)
RequestIDHeader(13-13)RequestIDKey(21-21)UsernameHeader(9-9)AuthAccountNameKey(20-20)
pkg/http/handler.go (3)
pkg/http/schema.go (1)
ApiError(11-16)pkg/portal/consts.go (8)
UsernameHeader(9-9)IntendedOriginHeader(14-14)TimestampHeader(11-11)NonceHeader(12-12)TokenHeader(8-8)RequestIDKey(21-21)RequestIDHeader(13-13)AuthAccountNameKey(20-20)pkg/portal/support.go (1)
ParseClientIP(171-187)
⏰ 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 (1.25.1)
- GitHub Check: test (1.25.1)
🔇 Additional comments (25)
pkg/http/schema.go (2)
15-15: LGTM! Error wrapping field added correctly.The
Errfield withjson:"-"tag correctly enables error wrapping while preventing serialization issues.
26-32: LGTM! Unwrap implementation is correct.The nil-safe
Unwrap()method properly implements Go's error unwrapping convention, enablingerrors.Isanderrors.Ascompatibility.pkg/http/response.go (3)
95-103: LGTM! Error wrapping consistently applied.The logging helpers now correctly populate the
Errfield, enabling Sentry to capture the underlying exception.
112-120: LGTM! Consistent error wrapping.Matches the pattern in
LogInternalError.
122-130: LGTM! Consistent error wrapping.Matches the pattern in the other logging helpers.
pkg/http/schema_test.go (2)
3-6: LGTM! Import updated for error testing.The
errorspackage import supports the new unwrap test.
25-45: LGTM! Comprehensive unwrap test coverage.The test correctly verifies:
errors.Iscompatibility with wrapped errorsUnwrap()returns the underlying cause- Nil-safe behavior
pkg/http/handler_test.go (4)
3-13: LGTM! Imports updated for new tests.The added imports support testing of context handling, error chains, and portal constants.
42-55: LGTM! Request ID extraction test is thorough.The test correctly verifies that context values take precedence over headers, which is the expected behavior.
57-70: LGTM! Account name extraction test is thorough.The test correctly verifies the context-over-header priority, matching the pattern in
TestRequestIDFrom.
72-85: LGTM! Error chain test validates unwrapping.The test correctly verifies that
buildErrorChainrecursively unwraps errors and builds the expected chain.pkg/http/handler.go (14)
3-14: LGTM! Imports updated for enhanced functionality.The new imports support error unwrapping, metadata extraction, and portal constants integration.
49-52: LGTM! Correct error capture logic.Capturing
apiErr.Errwhen present ensures Sentry receives the underlying exception for better grouping and fingerprinting. Based on learnings.
60-63: LGTM! Request ID enrichment aids debugging.Adding request ID as both a tag (for filtering) and an extra (for context) follows Sentry best practices for correlation.
68-68: LGTM! Scope enrichment call enhances debugging.The new enrichment function adds valuable metadata for troubleshooting API errors.
70-70: LGTM! Captures the most specific error.Using
errToCapture(determined at lines 49-52) ensures Sentry receives the most precise error for analysis.
82-94: LGTM! Error cause enrichment is comprehensive.The error cause, cause type tag, and error chain provide excellent debugging context. The chain unwrapping correctly uses
errors.Unwrap.
96-103: LGTM! Account name enrichment aids troubleshooting.Extracting account name from both context and header provides valuable user context for debugging.
104-114: LGTM! Request metadata enrichment is helpful.The intended origin, timestamp, and nonce provide useful request context for debugging API issues.
120-122: LGTM! Client IP enrichment aids debugging.Using
portal.ParseClientIPcorrectly handles X-Forwarded-For and RemoteAddr for reliable client identification.
125-133: LGTM! Correct context-over-header priority.Checking context first ensures that validated/processed values from middleware take precedence over raw headers.
135-143: LGTM! Consistent extraction pattern.Matches the context-first pattern in
requestIDFrom.
145-147: LGTM! Simple and consistent header extraction.The trimming ensures clean values throughout the error enrichment.
149-157: LGTM! Correct error chain construction.The function properly unwraps errors using
errors.Unwrapand builds a complete chain. The capacity hint of 4 is reasonable for typical error chains.
116-118: LoggingX-API-Keyis safe:portal.TokenHeadermaps to the public API key (see pkg/portal/consts.go) and only carries the public token, not the secret—safe to record asapi_public_key.
gocanto
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/http/scope_api_error.go (1)
73-99: Enforce PII scrubbing for Sentry extras
- Disable
send_default_piiin your SDK unless strictly required.- Implement
before_send/before_send_transactionto remove or mask these extras:
• api_account_name
• api_username_header
• api_intended_origin
• api_request_timestamp
• api_request_nonce
• api_public_key
• http_client_ip- Enable server-side Data Scrubber (Project Settings → Security & Privacy → Additional Sensitive Fields) and add the above keys.
- Optionally deploy Relay for pre-ingestion filtering and verify your DPA/retention settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/http/handler.go(1 hunks)pkg/http/handler_test.go(2 hunks)pkg/http/scope_api_error.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/http/handler_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/http/scope_api_error.go (3)
pkg/http/schema.go (1)
ApiError(11-16)pkg/portal/consts.go (8)
RequestIDKey(21-21)RequestIDHeader(13-13)UsernameHeader(9-9)IntendedOriginHeader(14-14)TimestampHeader(11-11)NonceHeader(12-12)TokenHeader(8-8)AuthAccountNameKey(20-20)pkg/portal/support.go (1)
ParseClientIP(171-187)
pkg/http/handler.go (1)
pkg/http/scope_api_error.go (1)
NewScopeApiError(20-22)
⏰ 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). (1)
- GitHub Check: test (1.25.1)
🔇 Additional comments (9)
pkg/http/handler.go (2)
39-42: LGTM! Correct error selection for Sentry capture.The logic correctly prioritizes the underlying cause when present, ensuring Sentry receives the original exception rather than the wrapper. The type assertion
error(apiErr)is necessary for the fallback case.
46-50: LGTM! Effective enrichment delegation.The centralized enrichment approach via
ScopeApiErrorimproves maintainability. CapturingerrToCaptureinstead of the wrapper ensures Sentry gets the most precise error with full stack trace.pkg/http/scope_api_error.go (7)
14-22: LGTM! Clean type definition and constructor.The struct encapsulates the necessary dependencies, and the constructor follows Go conventions. Unexported fields appropriately restrict direct access.
24-36: LGTM! Robust request ID extraction.The fallback pattern (context value → header) is sound, and the nil checks prevent panics. Trimming whitespace ensures clean IDs.
43-51: LGTM! Appropriate severity levels.The level assignment correctly distinguishes client errors (warning) from server errors (error), aligning with standard HTTP semantics and Sentry best practices.
66-71: LGTM! Complete error cause capture.Capturing both the error string, type, and full chain provides comprehensive debugging context. The cause chain will always have at least one element since
apiErr.Erris non-nil here.
102-114: LGTM! Consistent value extraction pattern.The method mirrors
RequestID()with context-first, header-fallback logic and appropriate nil safety.
116-122: LGTM! Clean helper utility.The helper consistently handles nil checks and whitespace trimming across all header accesses.
124-132: LGTM! Correct error chain traversal.The implementation properly uses
errors.Unwrapto walk the error chain. Preallocating with capacity 4 is a reasonable optimization for typical error depths.
gocanto
deleted the
gus/add-debugging-information-for-http-errors-2025-10-15
branch
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
Summary
Testing
Summary by CodeRabbit