🥅 server: fingerprint service errors by name and message#834
🥅 server: fingerprint service errors by name and message#834cruzdanilo wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 6e7a38e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello @cruzdanilo, 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 refines the error reporting strategy, particularly for service-level errors. By leveraging error names and messages, it aims to improve the grouping and identification of recurring issues in Sentry, making error monitoring more effective and actionable. Highlights
Changelog
Activity
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
|
WalkthroughThis PR refactors error fingerprinting logic in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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
The pull request refactors error fingerprinting in Sentry to use ServiceError for more structured error reporting. It removes complex manual parsing of error messages in app.onError and introduces a ServiceError check to directly use error.name and error.message for fingerprinting. Additionally, it enhances the beforeSend hook in instrument.cjs to capture error.name and error.message for errors that have a status property but lack an existing fingerprint, and refactors the revert error handling for clarity. Overall, the changes improve the consistency and clarity of error reporting to Sentry.
| app.onError((error, c) => { | ||
| let fingerprint: string[] | undefined; | ||
| if (error instanceof Error) { | ||
| const message = error.message | ||
| .split("Error:") | ||
| .reduce((result, part) => (result ? `${result}Error:${part}` : part.trimStart()), ""); | ||
| const status = message.slice(0, 3); | ||
| const hasStatus = /^\d{3}$/.test(status); | ||
| const hasBodyFormat = message.length === 3 || message[3] === " "; | ||
| const body = hasBodyFormat && message.length > 3 ? message.slice(4).trim() : undefined; | ||
| if (hasStatus && hasBodyFormat) fingerprint = ["{{ default }}", status]; | ||
| if (hasStatus && hasBodyFormat && body) { | ||
| try { | ||
| const json = JSON.parse(body) as { code?: unknown; error?: unknown; message?: unknown }; | ||
| fingerprint = [ | ||
| "{{ default }}", | ||
| status, | ||
| ...("code" in json | ||
| ? [String(json.code)] | ||
| : typeof json.message === "string" | ||
| ? [json.message] | ||
| : typeof json.error === "string" | ||
| ? [json.error] | ||
| : [body]), | ||
| ]; | ||
| } catch { | ||
| fingerprint = ["{{ default }}", status, body]; | ||
| } | ||
| } | ||
| } | ||
| captureException(error, { level: "error", tags: { unhandled: true }, fingerprint }); |
There was a problem hiding this comment.
The removal of the complex error message parsing logic is a significant improvement. Relying on ServiceError for structured error data simplifies the onError handler and makes error fingerprinting more robust and maintainable. This change directly addresses the goal of fingerprinting service errors by name and message, making the Sentry events more actionable.
| captureException(error, { | ||
| level: "error", | ||
| tags: { unhandled: true }, | ||
| fingerprint: error instanceof ServiceError ? ["{{ default }}", error.name, error.message] : undefined, | ||
| }); |
There was a problem hiding this comment.
This updated captureException call is much cleaner and leverages the ServiceError class effectively. It directly uses error.name and error.message for fingerprinting, which is a more reliable and structured approach than the previous manual parsing.
| ], | ||
| beforeSend: (event, hint) => { | ||
| const exception = event.exception?.values?.[0]; | ||
| if (!exception) return event; |
There was a problem hiding this comment.
Adding a check for !exception at the beginning of beforeSend ensures that subsequent logic only runs when there's an actual exception to process, preventing potential undefined errors and improving robustness.
| const error = hint.originalException; | ||
| if ( | ||
| error instanceof Error && | ||
| typeof (/** @type {{ status?: unknown }} */ (error).status) === "number" && | ||
| !(event.fingerprint && event.fingerprint.length > 1) | ||
| ) { | ||
| event.fingerprint = ["{{ default }}", error.name, error.message]; | ||
| } |
There was a problem hiding this comment.
This new block correctly identifies errors that have a status property (likely ServiceError instances or similar) and assigns a default fingerprint using error.name and error.message if one isn't already present. This ensures consistent fingerprinting for these types of errors, aligning with the PR's goal.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const error = hint.originalException; | ||
| if ( | ||
| error instanceof Error && | ||
| typeof (/** @type {{ status?: unknown }} */ (error).status) === "number" && |
There was a problem hiding this comment.
Bug: The new fingerprinting logic in instrument.cjs checks for error.status, but PandaError uses statusCode, causing these errors to be missed and not grouped correctly in Sentry.
Severity: MEDIUM
Suggested Fix
Update the check in server/instrument.cjs to look for error.statusCode in addition to error.status, or change the PandaError class to use a status property instead of statusCode for consistency.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: server/instrument.cjs#L32-L35
Potential issue: The fingerprinting logic in `server/instrument.cjs` is designed to
create custom Sentry fingerprints for errors that have a numeric `status` property.
However, `PandaError` instances, which are used for card transactions and refunds, are
defined with a `statusCode` property instead. When a `PandaError` is thrown and
captured, it is sent to Sentry without a pre-defined fingerprint. The `beforeSend` hook
then fails to apply a custom fingerprint because its check for `error.status` fails.
This results in `PandaError` events not being grouped correctly in Sentry, hindering
monitoring and debugging efforts.
Did we get this right? 👍 / 👎 to inform future reviews.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if ( | ||
| error instanceof Error && | ||
| typeof (/** @type {{ status?: unknown }} */ (error).status) === "number" && | ||
| !(event.fingerprint && event.fingerprint.length > 1) | ||
| ) { | ||
| event.fingerprint = ["{{ default }}", error.name, error.message]; | ||
| } |
There was a problem hiding this comment.
🚩 beforeSend fingerprinting applies to all errors with numeric status, not just ServiceError
The new beforeSend logic at server/instrument.cjs:33-38 fingerprints any Error with a numeric status property, which is broader than just ServiceError. This means if any third-party library throws an error with a numeric status (e.g., HTTP client libraries, ORMs), it will also get fingerprinted by ["{{ default }}", error.name, error.message]. If error.message contains high-cardinality data (request IDs, timestamps, UUIDs), this could create excessive unique fingerprints in Sentry, degrading grouping quality. ServiceError controls this via its constructor's parsing logic (falls back to status code for long messages >200 chars at server/utils/ServiceError.ts:26), but other error types have no such protection. This may be intentional to cast a wide net, but worth confirming.
Was this helpful? React with 👍 or 👎 to provide feedback.
| app.onError((error, c) => { | ||
| let fingerprint: string[] | undefined; | ||
| if (error instanceof Error) { | ||
| const message = error.message | ||
| .split("Error:") | ||
| .reduce((result, part) => (result ? `${result}Error:${part}` : part.trimStart()), ""); | ||
| const status = message.slice(0, 3); | ||
| const hasStatus = /^\d{3}$/.test(status); | ||
| const hasBodyFormat = message.length === 3 || message[3] === " "; | ||
| const body = hasBodyFormat && message.length > 3 ? message.slice(4).trim() : undefined; | ||
| if (hasStatus && hasBodyFormat) fingerprint = ["{{ default }}", status]; | ||
| if (hasStatus && hasBodyFormat && body) { | ||
| try { | ||
| const json = JSON.parse(body) as { code?: unknown; error?: unknown; message?: unknown }; | ||
| fingerprint = [ | ||
| "{{ default }}", | ||
| status, | ||
| ...("code" in json | ||
| ? [String(json.code)] | ||
| : typeof json.message === "string" | ||
| ? [json.message] | ||
| : typeof json.error === "string" | ||
| ? [json.error] | ||
| : [body]), | ||
| ]; | ||
| } catch { | ||
| fingerprint = ["{{ default }}", status, body]; | ||
| } | ||
| } | ||
| } | ||
| captureException(error, { level: "error", tags: { unhandled: true }, fingerprint }); | ||
| captureException(error, { | ||
| level: "error", | ||
| tags: { unhandled: true }, | ||
| fingerprint: error instanceof ServiceError ? ["{{ default }}", error.name, error.message] : undefined, | ||
| }); | ||
| return c.json({ code: "unexpected error", legacy: "unexpected error" }, 555 as UnofficialStatusCode); |
There was a problem hiding this comment.
🚩 Loss of fingerprinting for non-ServiceError status-formatted errors reaching app.onError
The old index.ts app.onError handler parsed error messages matching the pattern NNN body (e.g., "404 Not Found") and fingerprinted them with the extracted status code and optionally parsed JSON fields. The new code only fingerprints ServiceError instances. Any non-ServiceError Error that previously matched the old pattern (e.g., a plain Error('500 Internal Server Error')) will no longer be fingerprinted at the captureException call site. However, the beforeSend in instrument.cjs compensates for errors with a numeric status property. Plain Error objects without a status property that had status-formatted messages will lose their fingerprinting. This is likely acceptable since the codebase has been migrated to use ServiceError for service call failures, but it's a behavioral change.
Was this helpful? React with 👍 or 👎 to provide feedback.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #834 +/- ##
==========================================
+ Coverage 68.95% 69.00% +0.04%
==========================================
Files 211 211
Lines 7444 7455 +11
Branches 2385 2391 +6
==========================================
+ Hits 5133 5144 +11
Misses 2096 2096
Partials 215 215
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| app.onError((error, c) => { | ||
| let fingerprint: string[] | undefined; | ||
| if (error instanceof Error) { | ||
| const message = error.message | ||
| .split("Error:") | ||
| .reduce((result, part) => (result ? `${result}Error:${part}` : part.trimStart()), ""); | ||
| const status = message.slice(0, 3); | ||
| const hasStatus = /^\d{3}$/.test(status); | ||
| const hasBodyFormat = message.length === 3 || message[3] === " "; | ||
| const body = hasBodyFormat && message.length > 3 ? message.slice(4).trim() : undefined; | ||
| if (hasStatus && hasBodyFormat) fingerprint = ["{{ default }}", status]; | ||
| if (hasStatus && hasBodyFormat && body) { | ||
| try { | ||
| const json = JSON.parse(body) as { code?: unknown; error?: unknown; message?: unknown }; | ||
| fingerprint = [ | ||
| "{{ default }}", | ||
| status, | ||
| ...("code" in json | ||
| ? [String(json.code)] | ||
| : typeof json.message === "string" | ||
| ? [json.message] | ||
| : typeof json.error === "string" | ||
| ? [json.error] | ||
| : [body]), | ||
| ]; | ||
| } catch { | ||
| fingerprint = ["{{ default }}", status, body]; | ||
| } | ||
| } | ||
| } | ||
| captureException(error, { level: "error", tags: { unhandled: true }, fingerprint }); | ||
| captureException(error, { | ||
| level: "error", | ||
| tags: { unhandled: true }, | ||
| fingerprint: error instanceof ServiceError ? ["{{ default }}", error.name, error.message] : undefined, | ||
| }); |
There was a problem hiding this comment.
Add trailing comma in fingerprint array.
Line 280’s array should include a trailing comma per repo style.
🔧 Proposed fix
- fingerprint: error instanceof ServiceError ? ["{{ default }}", error.name, error.message] : undefined,
+ fingerprint: error instanceof ServiceError ? ["{{ default }}", error.name, error.message,] : undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.onError((error, c) => { | |
| let fingerprint: string[] | undefined; | |
| if (error instanceof Error) { | |
| const message = error.message | |
| .split("Error:") | |
| .reduce((result, part) => (result ? `${result}Error:${part}` : part.trimStart()), ""); | |
| const status = message.slice(0, 3); | |
| const hasStatus = /^\d{3}$/.test(status); | |
| const hasBodyFormat = message.length === 3 || message[3] === " "; | |
| const body = hasBodyFormat && message.length > 3 ? message.slice(4).trim() : undefined; | |
| if (hasStatus && hasBodyFormat) fingerprint = ["{{ default }}", status]; | |
| if (hasStatus && hasBodyFormat && body) { | |
| try { | |
| const json = JSON.parse(body) as { code?: unknown; error?: unknown; message?: unknown }; | |
| fingerprint = [ | |
| "{{ default }}", | |
| status, | |
| ...("code" in json | |
| ? [String(json.code)] | |
| : typeof json.message === "string" | |
| ? [json.message] | |
| : typeof json.error === "string" | |
| ? [json.error] | |
| : [body]), | |
| ]; | |
| } catch { | |
| fingerprint = ["{{ default }}", status, body]; | |
| } | |
| } | |
| } | |
| captureException(error, { level: "error", tags: { unhandled: true }, fingerprint }); | |
| captureException(error, { | |
| level: "error", | |
| tags: { unhandled: true }, | |
| fingerprint: error instanceof ServiceError ? ["{{ default }}", error.name, error.message] : undefined, | |
| }); | |
| app.onError((error, c) => { | |
| captureException(error, { | |
| level: "error", | |
| tags: { unhandled: true }, | |
| fingerprint: error instanceof ServiceError ? ["{{ default }}", error.name, error.message,] : undefined, | |
| }); |
| if (!exception) return event; | ||
| const error = hint.originalException; | ||
| if ( | ||
| error instanceof Error && | ||
| typeof (/** @type {{ status?: unknown }} */ (error).status) === "number" && | ||
| !(event.fingerprint && event.fingerprint.length > 1) | ||
| ) { | ||
| event.fingerprint = ["{{ default }}", error.name, error.message]; | ||
| } |
There was a problem hiding this comment.
Remove JSDoc cast and add trailing comma.
JSDoc casts are disallowed, and the fingerprint array needs a trailing comma.
🔧 Proposed fix
- if (
- error instanceof Error &&
- typeof (/** `@type` {{ status?: unknown }} */ (error).status) === "number" &&
- !(event.fingerprint && event.fingerprint.length > 1)
- ) {
- event.fingerprint = ["{{ default }}", error.name, error.message];
- }
+ if (
+ error instanceof Error &&
+ typeof error.status === "number" &&
+ !(event.fingerprint && event.fingerprint.length > 1)
+ ) {
+ event.fingerprint = ["{{ default }}", error.name, error.message,];
+ }1 participant
Greptile Summary
replaces complex error message parsing logic with clean
ServiceErrortype checking for sentry fingerprinting. the old approach parsed error messages to extract http status codes and json payloads (splitting on "Error:", regex matching status codes, json parsing). the new approach usesServiceError.nameandServiceError.messagedirectly when available, and falls back to generic fingerprinting for any error with astatusproperty ininstrument.cjs.server/index.tsServiceErrorbeforeSendhook ininstrument.cjsto fingerprint errors withstatusproperty usingerror.nameanderror.messageerrortorevertin the contract error handling loop to avoid variable shadowingConfidence Score: 5/5
Important Files Changed
ServiceErrorinstance check for error fingerprintingstatusproperty, renames variable to avoid shadowingLast reviewed commit: 6e7a38e
Summary by CodeRabbit