feat: crash reporter settings #393

cristianoventura · 2024-12-31T16:06:08Z

Description

This PR depends on #365. It adds a new option to the Settings dialog that allows users to opt out of k6 Studio sending error/crash reports to Sentry.

How to Test

manually throw an error anywhere in the application that is triggered on user click (e.g. toggle theme)
toggle the setting and check that the error event is not sent when the setting is disabled

Checklist

I have performed a self-review of my code.
I have added tests for my changes.
I have run linter locally (npm run lint) and all checks pass.
I have run tests locally (npm test) and all tests pass.
I have commented on my code, particularly in hard-to-understand areas.

Screenshots (if appropriate):

Related PR(s)/Issue(s)

…eporter

cristianoventura · 2024-12-31T16:07:06Z

src/components/Settings/TelemetrySettings.tsx

+                onCheckedChange={field.onChange}
+              />{' '}
+              Send crash reports and error data to Grafana to help improve k6
+              Studio. <Link href="">Learn more.</Link>


We'll likely need a link here leading to a doc similar to the usage collection.

Should we remove the link until we have the doc? Currently, it reloads the app

I was waiting for feedback and thinking that we could potentially have that link before releasing this feature. I just removed it and we can add it back later.

cristianoventura · 2024-12-31T16:07:57Z

src/main.ts

+    beforeSend: (event) => {
+      if (appSettings.telemetry.errorReport) {
+        return event
+      }


Sentry is always initialized in Prod mode, but the event is dynamically sent based on the errorReport setting.

cristianoventura · 2024-12-31T16:09:33Z

src/schemas/settings/v1/index.ts

+    telemetry: {
+      usageReport: settings.usageReport.enabled,
+      errorReport: true,
+    },


A migration was needed since usageReport was renamed to `telemetry

Don't we need to bump schema version, copy schema to v3 and add the adjustments there?
Everything works now, but if we introduce another change that depends on useageReport or telemetry and the user has skipped one version between updates - the migration won't work anymore because we lose that middle step.

You're right! I thought we hadn't released that change yet but it looks like v2 is the latest one in prod.

e-fisher

Added comments about migration and empty link

…studio into feat/crash-reporter-settings

going-confetti

In my opinion, the checkbox label/description should give the user a better understanding of where the data is sent, especially since we don't have the docs yet

src/components/Settings/TelemetrySettings.tsx

going-confetti · 2025-01-07T15:21:40Z

src/schemas/settings/v3/index.ts

+}
+
+export const AppSettingsSchema = z.object({
+  version: z.literal('3.0'),


It's probably too late to ask this, but in which cases would we bump the "minor" version? Maybe it's unnecessary and could be omitted from this version on?

Since all migrations are required (and likely cause breaking changes) and automatically fast-forwarded, I don't think bumping the minor would ever be suitable 🤔. We could use timestamps instead, like other major frameworks do. This change could also be incorporated into #392 where the migration name and version are automatically generated.

We could use timestamps instead, like other major frameworks do. This change could also be incorporated into #392 where the migration name and version are automatically generated.

I like this idea!

e-fisher · 2025-01-07T15:21:46Z

src/main.ts

+
+    // conditionally send the event based on the user's settings
+    beforeSend: (event) => {
+      if (appSettings.telemetry.errorReport) {


I remember writing a comment about this, but can't find anymore, probably forgot to submit it, sorry about that 😅

appSettings could be undefined, we need to update type to be AppSettings | undefined where is declared and add checks here. I've added a throw to appReady callback before appSettings variable is initialized and got this exception

Good catch! I chose to initialize the variable with the defaultSettings object instead. If we leave it as undefined, we'd need to perform additional type checks in multiple places, which could increase the overall maintenance effort.

e-fisher

LGTM 🚀

going-confetti · 2025-01-09T09:47:07Z

src/main.ts

@@ -79,7 +88,7 @@ const PROXY_RETRY_LIMIT = 5
 let proxyRetryCount = 0
 let currentClientRoute = '/'
 let wasAppClosedByClient = false
-export let appSettings: AppSettings
+export let appSettings = defaultSettings


Not sure how I feel about this, as it can lead to a situation when defaultSettings would be used instead of the actual settings. I see that it's only imported in browser.ts - can we add detectBrowserPath as a parameter to launchBrowser instead?

Not sure how I feel about this, as it can lead to a situation when defaultSettings would be used instead of the actual settings

@going-confetti This already happens. The getSettings method returns the default object if the settings file can't be parsed. It's a fault tolerance mechanism. appSettings will be set at some point (either to the actual settings or failover to default)

k6-studio/src/settings.ts

Lines 68 to 72 in ff74970

} catch (error) {

log.error('Failed to parse settings file', error)

// if the file is invalid during runtime,

// return a valid settings object so the app can keep running

return defaultSettings

I see that it's only imported in browser.ts - can we add detectBrowserPath as a parameter to launchBrowser instead?

This is a good idea! There will be probably additional places where we'll have to use appSettings with the separation of modules that we plan to work on #378, so I'll defer this change to when we get there.

This already happens. The getSettings method returns the default object if the settings file can't be parsed. It's a fault tolerance mechanism. appSettings will be set at some point (either to the actual settings or failover to default)

IMO it could be a big deal if the settings can't be parsed and we use default settings as a fallback without informing the user. For example, if they have custom proxy configuration and stuff suddenly stops working.
Seems out of scope of this PR though

Agreed! We can work on something to let users know that the default config was loaded if the parsing fails.

cristianoventura added 9 commits November 27, 2024 09:59

send sourcemap to sentry

40490f8

initialize sentry

537250a

Lint

dfaac67

add sentry org and projec variables to release.yml

bd84364

Merge branch 'main' of github.com:grafana/k6-studio into feat/crash-r…

93f2055

…eporter

Merge branch 'main' of github.com:grafana/k6-studio into feat/crash-r…

11996a9

…eporter

crash report settings

72c81a3

conditionally send event based on user preference

07c31d2

temporarily remove link from sentry opt-in

43631c7

cristianoventura self-assigned this Dec 31, 2024

cristianoventura commented Dec 31, 2024

View reviewed changes

cristianoventura marked this pull request as ready for review January 2, 2025 16:31

cristianoventura requested a review from a team as a code owner January 2, 2025 16:31

cristianoventura requested review from going-confetti, e-fisher and Llandy3d and removed request for a team January 2, 2025 16:31

cristianoventura mentioned this pull request Jan 3, 2025

feat: implement crash reporter #365

Merged

5 tasks

e-fisher reviewed Jan 6, 2025

View reviewed changes

cristianoventura added 2 commits January 7, 2025 06:38

Remove learn more link

989c43f

Bump schema version

88c205d

cristianoventura requested a review from e-fisher January 7, 2025 15:08

Base automatically changed from feat/crash-reporter to main January 7, 2025 15:08

cristianoventura added 4 commits January 7, 2025 10:09

Merge branch 'main' into feat/crash-reporter-settings

a425d5d

Fix test

879fa3c

Merge branch 'feat/crash-reporter-settings' of github.com:grafana/k6-…

494642a

…studio into feat/crash-reporter-settings

Test v2 migration

0f60ba8

going-confetti requested changes Jan 7, 2025

View reviewed changes

Change copy for Telemetry settings

ee8b656

cristianoventura requested a review from going-confetti January 7, 2025 19:38

cristianoventura mentioned this pull request Jan 7, 2025

feat: enable Sentry in renderer process #397

Open

5 tasks

e-fisher reviewed Jan 8, 2025

View reviewed changes

initialize appSettings with default values

18c18ad

cristianoventura requested a review from e-fisher January 8, 2025 17:19

e-fisher approved these changes Jan 9, 2025

View reviewed changes

going-confetti reviewed Jan 9, 2025

View reviewed changes

cristianoventura requested a review from going-confetti January 9, 2025 14:51

going-confetti approved these changes Jan 9, 2025

View reviewed changes

cristianoventura merged commit 898bde8 into main Jan 9, 2025
2 checks passed

cristianoventura deleted the feat/crash-reporter-settings branch January 9, 2025 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: crash reporter settings #393

feat: crash reporter settings #393

cristianoventura commented Dec 31, 2024

cristianoventura Dec 31, 2024

e-fisher Jan 6, 2025

cristianoventura Jan 7, 2025 •

edited

Loading

cristianoventura Dec 31, 2024

cristianoventura Dec 31, 2024

e-fisher Jan 6, 2025

cristianoventura Jan 7, 2025

e-fisher left a comment

going-confetti left a comment

going-confetti Jan 7, 2025

cristianoventura Jan 7, 2025

going-confetti Jan 9, 2025

e-fisher Jan 7, 2025

cristianoventura Jan 8, 2025

e-fisher left a comment

going-confetti Jan 9, 2025

cristianoventura Jan 9, 2025

going-confetti Jan 9, 2025

cristianoventura Jan 9, 2025

	} catch (error) {
	log.error('Failed to parse settings file', error)
	// if the file is invalid during runtime,
	// return a valid settings object so the app can keep running
	return defaultSettings

feat: crash reporter settings #393

feat: crash reporter settings #393

Conversation

cristianoventura commented Dec 31, 2024

Description

How to Test

Checklist

Screenshots (if appropriate):

Related PR(s)/Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoventura Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-fisher left a comment

Choose a reason for hiding this comment

going-confetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-fisher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoventura Jan 7, 2025 •

edited

Loading