Split CLI error analytics grouping#7894
Conversation
stephanie-shopify
left a comment
stephanie-shopify
left a comment
There was a problem hiding this comment.
Review focused on one critical behavior change plus a few correctness/cleanup items. The grouping mechanism itself is well-designed and the GraphQL normalization targets the right root cause — main blocker is the silent removal of expected-error suppression (see inline). Posting as comments since this is my own PR.
| } | ||
|
|
||
| // If the error was unexpected, we flag it as "unhandled" in Bugsnag. This is a helpful distinction. | ||
| const unhandled = exitMode === 'unexpected_error' |
There was a problem hiding this comment.
🚨 Critical / not in the summary: the diff removes the if (exitMode === 'expected_error') { return {reported:false…} } guard that used to sit right above this line, and flips the test from 'when error is expected' (not reported) to 'processes AbortErrors as handled' (reported: true).
Net effect: every AbortError now flows to Bugsnag as a handled event — these were deliberately suppressed before. This is unrelated to grouping, increases Bugsnag volume/cost by an unquantified amount, and runs opposite to the linked issue's own recommendation ("stop reporting known transient/user errors … so they leave crash reporting entirely").
Please either (a) revert this removal and keep the PR purely about grouping, or (b) split it out, justify it with projected event volume, and document it in both the summary and the changeset (which currently only mentions grouping). As written it's the riskiest line in the diff and the least visible.
| const {commandStartOptions} = metadata.getAllSensitiveMetadata() | ||
| const {startCommand} = commandStartOptions ?? {} | ||
| const sliceName = sliceNameFromStartCommand(startCommand) | ||
| event.groupingHash = bugsnagGroupingHash(reportableError, sliceName) |
There was a problem hiding this comment.
Setting groupingHash unconditionally re-buckets all CLI errors (known, acknowledged). The sharp edge is the Unknown category: formatGenericError keys on the first 50 slugified chars, so two genuinely distinct crashes in the same slice that share an opening message fragment will now merge into one bucket, discarding the stack-trace grouping that keeps them apart today.
Consider skipping the override when category === Unknown (let Bugsnag's stack grouping stand), or folding a short stack fingerprint into the hash for that case. That keeps the win for categorized buckets without coarsening the truly-unknown ones.
| if (response.status === 401 && !firstErrorMessage?.match(/auth|credential|token|unauthorized/i)) { | ||
| parts.push('Unauthorized') | ||
| } | ||
| if (response.status === 403 && !firstErrorMessage?.match(/access|denied|forbidden|permission|unauthorized/i)) { |
There was a problem hiding this comment.
'Forbidden' is misleading given the taxonomy: 'forbidden' is an Authentication term, not a Permission term (['unauthorized','forbidden','auth','token','credential'] vs ['permission','denied','access','insufficient']). So a 403 with a generic message lands in :authentication:, even though 403 = permission semantically — and the issue explicitly frames App-Management 403 as a permission problem distinct from 401 auth.
Decide explicitly: if 403 should be permission, push 'access denied' instead of 'Forbidden'. If everything 4xx→authentication is fine, drop the misleading word and add a comment. Either way there's no test for the 403-with-JSON-payload path, so current behavior is unverified.
There was a problem hiding this comment.
403 can be permission instead of forbidden
|
|
||
| const firstError = response.errors?.[0] | ||
| const code = firstError?.extensions?.code | ||
| const firstErrorMessage = firstError?.message?.replace(/too many requests/gi, 'rate limit exceeded') |
There was a problem hiding this comment.
💡 'too many requests' is already a RateLimit term, so this rewrite doesn't change categorization — both forms categorize as rate_limit. If the goal is just a tidier signature, a one-line comment would help; otherwise it's removable.
| const graphQLError = graphQLErrorForGrouping(error.message) | ||
| if (graphQLError) return graphQLError | ||
|
|
||
| const messageWithoutJsonDump = error.message.split(': {')[0] ?? error.message |
There was a problem hiding this comment.
💡 String.prototype.split always returns a non-empty array, so [0] is never undefined and the ?? error.message fallback is unreachable. Same dead fallback on the return new Error(... ?? message) line below. Harmless, just drop for clarity.
| : path.joinPath(projectRoot, currentFilePath) | ||
|
|
||
| const matchingPluginPath = pluginLocations.find(({pluginPath}) => fullLocation.startsWith(pluginPath)) | ||
| const matchingPluginPath = pluginLocations.filter(({pluginPath}) => fullLocation.startsWith(pluginPath))[0] |
There was a problem hiding this comment.
💡 Unrelated change, and strictly worse: .find(pred) short-circuits on first match; .filter(pred)[0] walks the whole array and allocates a throwaway array for an identical result. Recommend reverting to .find(...) unless there's a reason I'm missing.
|
generally dont like this approach. feels brittle to have the hard coded categories and there are some weird unnecessary changes. going back to drawing board |
1 participant
Summary
closes #7891 which stems from this resiliency issue which is currently an aggregate of a bunch of different issues so we can never properly address it
Testing
pnpm vitest run packages/cli-kit/src/public/node/error-handler.test.ts(passed in primary checkout)pnpm nx type-check cli-kit(passed in primary checkout)pnpm nx lint cli-kit(passed in primary checkout; existing warnings only)context
https://shopify.slack.com/archives/C0AFPGQJENQ/p1782147409267669