-
Notifications
You must be signed in to change notification settings - Fork 1k
allow WRANGLER_SEND_ERROR_REPORTS to override whether to report Wrangler crashes to Sentry #10735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "wrangler": patch | ||
| --- | ||
|
|
||
| Allow WRANGLER_SEND_ERROR_REPORTS env var to override whether to report Wrangler crashes to Sentry | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -123,6 +123,31 @@ describe("sentry", () => { | |||||
| expect(sentryRequests?.length).toEqual(0); | ||||||
| }); | ||||||
|
|
||||||
| it("should not hit sentry (or even ask) after reportable error if WRANGLER_SEND_ERROR_REPORTS is explicitly false", async () => { | ||||||
| // Trigger an API error | ||||||
| msw.use( | ||||||
| http.get( | ||||||
| `https://api.cloudflare.com/client/v4/user`, | ||||||
| async () => { | ||||||
| return HttpResponse.error(); | ||||||
| }, | ||||||
| { once: true } | ||||||
| ), | ||||||
| http.get("*/user/tokens/verify", () => { | ||||||
| return HttpResponse.json(createFetchResult([])); | ||||||
| }) | ||||||
| ); | ||||||
| await expect( | ||||||
| runWrangler("whoami", { WRANGLER_SEND_ERROR_REPORTS: "false" }) | ||||||
| ).rejects.toMatchInlineSnapshot(`[TypeError: Failed to fetch]`); | ||||||
| expect(std.out).toMatchInlineSnapshot(` | ||||||
| "Getting User settings... | ||||||
|
|
||||||
| [32mIf you think this is a bug then please create an issue at https://github.com/cloudflare/workers-sdk/issues/new/choose[0m" | ||||||
| `); | ||||||
| expect(sentryRequests?.length).toEqual(0); | ||||||
| }); | ||||||
|
|
||||||
| it("should hit sentry after reportable error when permission provided", async () => { | ||||||
| // Trigger an API error | ||||||
| msw.use( | ||||||
|
|
@@ -428,6 +453,308 @@ describe("sentry", () => { | |||||
| }, | ||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| it("should hit sentry after reportable error (without confirmation) if WRANGLER_SEND_ERROR_REPORTS is explicitly true", async () => { | ||||||
| // Trigger an API error | ||||||
| msw.use( | ||||||
| http.get( | ||||||
| `https://api.cloudflare.com/client/v4/user`, | ||||||
| async () => { | ||||||
| return HttpResponse.error(); | ||||||
| }, | ||||||
| { once: true } | ||||||
| ), | ||||||
| http.get("*/user/tokens/verify", () => { | ||||||
| return HttpResponse.json(createFetchResult([])); | ||||||
| }) | ||||||
| ); | ||||||
| await expect( | ||||||
| runWrangler("whoami", { WRANGLER_SEND_ERROR_REPORTS: "true" }) | ||||||
| ).rejects.toMatchInlineSnapshot(`[TypeError: Failed to fetch]`); | ||||||
| expect(std.out).toMatchInlineSnapshot(` | ||||||
| "Getting User settings... | ||||||
|
|
||||||
| [32mIf you think this is a bug then please create an issue at https://github.com/cloudflare/workers-sdk/issues/new/choose[0m" | ||||||
| `); | ||||||
|
|
||||||
| // Sentry sends multiple HTTP requests to capture breadcrumbs | ||||||
| expect(sentryRequests?.length).toBeGreaterThan(0); | ||||||
| assert(sentryRequests !== undefined); | ||||||
|
|
||||||
| // Check requests don't include PII | ||||||
| const envelopes = sentryRequests.map(({ envelope }) => { | ||||||
| const parts = envelope.split("\n").map((line) => JSON.parse(line)); | ||||||
| expect(parts).toHaveLength(3); | ||||||
| return { header: parts[0], type: parts[1], data: parts[2] }; | ||||||
| }); | ||||||
| const event = envelopes.find(({ type }) => type.type === "event"); | ||||||
| assert(event !== undefined); | ||||||
|
|
||||||
| // Redact fields with random contents we know don't contain PII | ||||||
| event.header.event_id = ""; | ||||||
| event.header.sent_at = ""; | ||||||
| event.header.trace.trace_id = ""; | ||||||
| event.header.trace.release = ""; | ||||||
| for (const exception of event.data.exception.values) { | ||||||
| for (const frame of exception.stacktrace.frames) { | ||||||
| if ( | ||||||
| frame.filename.startsWith("C:\\Project\\") || | ||||||
| frame.filename.startsWith("/project/") | ||||||
| ) { | ||||||
| frame.filename = "/project/..."; | ||||||
| } | ||||||
| frame.function = ""; | ||||||
| frame.lineno = 0; | ||||||
| frame.colno = 0; | ||||||
| frame.in_app = false; | ||||||
| frame.pre_context = []; | ||||||
| frame.context_line = ""; | ||||||
| frame.post_context = []; | ||||||
| } | ||||||
| } | ||||||
| event.data.event_id = ""; | ||||||
| event.data.contexts.trace.trace_id = ""; | ||||||
| event.data.contexts.trace.span_id = ""; | ||||||
| event.data.contexts.runtime.version = ""; | ||||||
| event.data.contexts.app.app_start_time = ""; | ||||||
| event.data.contexts.app.app_memory = 0; | ||||||
| event.data.contexts.os = {}; | ||||||
| event.data.contexts.device = {}; | ||||||
| event.data.timestamp = 0; | ||||||
| event.data.release = ""; | ||||||
| for (const breadcrumb of event.data.breadcrumbs) { | ||||||
| breadcrumb.timestamp = 0; | ||||||
| } | ||||||
|
|
||||||
| const fakeInstallPath = "/wrangler/"; | ||||||
| for (const exception of event.data.exception?.values ?? []) { | ||||||
| for (const frame of exception.stacktrace?.frames ?? []) { | ||||||
| if (frame.module.startsWith("@mswjs")) { | ||||||
| frame.module = | ||||||
| "@mswjs.interceptors.src.interceptors.fetch:index.ts"; | ||||||
| } | ||||||
| if (frame.filename === undefined) { | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| const wranglerPackageIndex = frame.filename.indexOf( | ||||||
| path.join("packages", "wrangler", "src") | ||||||
| ); | ||||||
| if (wranglerPackageIndex === -1) { | ||||||
| continue; | ||||||
| } | ||||||
| frame.filename = | ||||||
| fakeInstallPath + | ||||||
| frame.filename | ||||||
| .substring(wranglerPackageIndex) | ||||||
| .replaceAll("\\", "/"); | ||||||
| continue; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // If more data is included in the Sentry request, we'll need to verify it | ||||||
| // couldn't contain PII and update this snapshot | ||||||
| expect(event).toStrictEqual({ | ||||||
| data: { | ||||||
| breadcrumbs: [ | ||||||
| { | ||||||
| level: "log", | ||||||
| message: "wrangler whoami", | ||||||
| timestamp: 0, | ||||||
| }, | ||||||
| ], | ||||||
| contexts: { | ||||||
| app: { | ||||||
| app_memory: 0, | ||||||
| app_start_time: "", | ||||||
| }, | ||||||
| cloud_resource: {}, | ||||||
| device: {}, | ||||||
| os: {}, | ||||||
| runtime: { | ||||||
| name: "node", | ||||||
| version: "", | ||||||
| }, | ||||||
| trace: { | ||||||
| span_id: "", | ||||||
| trace_id: "", | ||||||
| }, | ||||||
| }, | ||||||
| environment: "production", | ||||||
| event_id: "", | ||||||
| exception: { | ||||||
| values: [ | ||||||
| { | ||||||
| mechanism: { | ||||||
| handled: true, | ||||||
| type: "generic", | ||||||
| }, | ||||||
| stacktrace: { | ||||||
| frames: [ | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: expect.any(String), | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: expect.any(String), | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: expect.any(String), | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: expect.any(String), | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: expect.any(String), | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: expect.any(String), | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: expect.any(String), | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: expect.any(String), | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: expect.any(String), | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: expect.any(String), | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: expect.any(String), | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: expect.any(String), | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: expect.any(String), | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: expect.any(String), | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: expect.any(String), | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: expect.any(String), | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: "/project/...", | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: | ||||||
| "@mswjs.interceptors.src.interceptors.fetch:index.ts", | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| { | ||||||
| colno: 0, | ||||||
| context_line: "", | ||||||
| filename: "/project/...", | ||||||
| function: "", | ||||||
| in_app: false, | ||||||
| lineno: 0, | ||||||
| module: | ||||||
| "@mswjs.interceptors.src.interceptors.fetch:index.ts", | ||||||
| post_context: [], | ||||||
| pre_context: [], | ||||||
| }, | ||||||
| ], | ||||||
| }, | ||||||
| type: "TypeError", | ||||||
| value: "Failed to fetch", | ||||||
| }, | ||||||
| ], | ||||||
| }, | ||||||
| modules: {}, | ||||||
| platform: "node", | ||||||
| release: "", | ||||||
| sdk: { | ||||||
| integrations: [ | ||||||
| "InboundFilters", | ||||||
| "FunctionToString", | ||||||
| "LinkedErrors", | ||||||
| "Console", | ||||||
| "OnUncaughtException", | ||||||
| "OnUnhandledRejection", | ||||||
| "ContextLines", | ||||||
| "Context", | ||||||
| "Modules", | ||||||
| ], | ||||||
| name: "sentry.javascript.node", | ||||||
| packages: [ | ||||||
| { | ||||||
| name: "npm:@sentry/node", | ||||||
| version: "7.87.0", | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| }, | ||||||
| ], | ||||||
| version: "7.87.0", | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| }, | ||||||
| timestamp: 0, | ||||||
| }, | ||||||
| header: { | ||||||
| event_id: "", | ||||||
| sdk: { | ||||||
| name: "sentry.javascript.node", | ||||||
| version: "7.87.0", | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will require us to update the test when ever we update the sentry client. Would it be better to match it with There are a few more version number above.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| }, | ||||||
| sent_at: "", | ||||||
| trace: { | ||||||
| environment: "production", | ||||||
| public_key: "9edbb8417b284aa2bbead9b4c318918b", | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like a magic number that could change? Maybe match it with
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| release: "", | ||||||
| trace_id: "", | ||||||
| }, | ||||||
| }, | ||||||
| type: { | ||||||
| type: "event", | ||||||
| }, | ||||||
| }); | ||||||
| }); | ||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a fix or new feature? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll bump it to minor