[wrangler] Reduce Sentry noise by classifying user-environment errors correctly#13835
[wrangler] Reduce Sentry noise by classifying user-environment errors correctly#13835
Conversation
… correctly Converted common user-environment failures (missing build outputs, expired tokens, transient network issues, C3 process exits, invalid API tokens, missing assets directories, framework version detection, container SSH, D1 export limitations, and inline source maps) to UserError so they are no longer reported as crashes in Sentry. Also tightened top-level error filtering to recognise duck-typed UserError, MiniflareError.isUserError(), a broader set of authentication errors, transient network errors, and filesystem environment errors.
🦋 Changeset detectedLatest commit: 2e149e9 The changes in this PR will be included in the next version bump. 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 |
Changeset ReviewSummary✅ All changesets look good Reviewed Changesets
Validation Results
|
workers-devprod
requested review from
a team and
emily-shen
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR reclassifies a large set of user-environment errors as UserError to reduce Sentry noise, tightens error filtering in the top-level handler, and adds new predicates for transient network, benign write, and filesystem environment errors.
Issues
-
Bug:
isCloudflareAPIDNSErrormixeseanderrinside thevisitErrorOrCausecallback — The visitor callback receives the currently-visited node aserr, but line 197 callshasErrorCode(e, ...)(the outer top-level error), and lines 203–204 check"hostname" in e(again the outere) while line 206 reads(err as { hostname }).hostname(the inner node). Whene.causehas theENOTFOUNDcode and hostname buteitself does not, the hostname check is evaluated against the wrong object. See suggestion below. -
Bug: dead
UserError-preserving branch instart-remote-proxy-session.tssecond catch —maybeErroris a{ error: unknown }wrapper, not anErrorinstance.maybeError instanceof UserErrorandmaybeError instanceof Errorare alwaysfalse, soErrorClassis alwaysErrorand theUserErrorbranch is unreachable. The check should inspectmaybeError.errorinstead.
|
Review posted successfully on PR #13835. I found two bugs:
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ssion.ts Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
/bonk fix the build errors on this PR |
|
The build error was a syntax error in The fix was simply removing the duplicate
The other review-flagged bug ( |
| } catch (e) { | ||
| if (e instanceof TypeError && String(e.message).includes("ByteString")) { | ||
| throw new UserError( | ||
| "Your API token contains an invalid character. API tokens must be plain text.", |
There was a problem hiding this comment.
include the header name in the error message?
| if (errorCode !== undefined) { | ||
| apiError.code = errorCode; | ||
| } | ||
| if (response.status >= 400 && response.status < 500) { |
There was a problem hiding this comment.
nit: we make this check a lot, pull it out into a helper?
| "code" in e && | ||
| (e as { code: string }).code === "ENOENT" | ||
| ) { | ||
| throw new NonExistentAssetsDirError( |
There was a problem hiding this comment.
not worth the noise, but i'd like to commemorate the one and only time a sentry alert actually led to me fixing a bug, after i released a dodgy C3 template and we got a bunch of errors here.
3 participants
Summary
Converted common user-environment failures to
UserErrorso they are no longer reported as crashes in Sentry. Also tightened top-level error filtering.Changes
init.ts— C3 child-process delegation failures →UserErrorstart-remote-proxy-session.ts— preservesUserErroridentity across catch blockscore/handle-errors.ts— duck-typedUserErrordetection, broader auth error matching (401/403), new filters for transient network errors, benign write errors, filesystem environment errors, and SSL certificate errorscfetch/internal.ts— invalid API token characters (ByteString errors) caught and converted toUserError; KV/R2 4xx responses useAPIError/UserErrorassets.ts— missing assets directory at upload time →NonExistentAssetsDirErrorautoconfig/frameworks/framework-class.ts— framework version detection failure →UserErrorcontainers/ssh.ts— SSH errors (instance not found, auth failures, exit 255) →UserErrord1/export.ts— D1 export limitations →UserErrordeployment-bundle/source-maps.ts— inline data-URL source maps →UserErrorAuthor has addressed the following