Skip to content
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

feat: crash reporter settings #393

Merged
merged 17 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/components/Settings/SettingsDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { ProxySettings } from './ProxySettings'
import { AppSettingsSchema } from '@/schemas/settings'
import { RecorderSettings } from './RecorderSettings'
import { AppSettings } from '@/types/settings'
import { UsageReportSettings } from './UsageReportSettings'
import { TelemetrySettings } from './TelemetrySettings'
import { ButtonWithTooltip } from '../ButtonWithTooltip'
import { AppearanceSettings } from './AppearanceSettings'
import { LogsSettings } from './LogsSettings'
Expand All @@ -25,9 +25,9 @@ const tabs = [
{ label: 'Proxy', value: 'proxy', component: ProxySettings },
{ label: 'Recorder', value: 'recorder', component: RecorderSettings },
{
label: 'Usage collection',
label: 'Telemetry',
value: 'usageReport',
component: UsageReportSettings,
component: TelemetrySettings,
},
{
label: 'Appearance',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Controller, useFormContext } from 'react-hook-form'
import { SettingsSection } from './SettingsSection'
import { AppSettings } from '@/types/settings'

export const UsageReportSettings = () => {
export const TelemetrySettings = () => {
const { control, register } = useFormContext<AppSettings>()

const handleLinkClick = () =>
Expand All @@ -13,14 +13,14 @@ export const UsageReportSettings = () => {

return (
<SettingsSection>
<Flex gap="2">
<Flex gap="2" mb="4">
<Controller
control={control}
name="usageReport.enabled"
name="telemetry.usageReport"
render={({ field }) => (
<Text size="2" as="label">
<Checkbox
{...register('usageReport.enabled')}
{...register('telemetry.usageReport')}
checked={field.value}
onCheckedChange={field.onChange}
/>{' '}
Expand All @@ -33,6 +33,24 @@ export const UsageReportSettings = () => {
)}
/>
</Flex>

<Flex gap="2">
<Controller
control={control}
name="telemetry.errorReport"
render={({ field }) => (
<Text size="2" as="label">
<Checkbox
{...register('telemetry.errorReport')}
checked={field.value}
onCheckedChange={field.onChange}
/>{' '}
Send crash reports and error data to Grafana to help improve k6
going-confetti marked this conversation as resolved.
Show resolved Hide resolved
Studio.
</Text>
)}
/>
</Flex>
</SettingsSection>
)
}
16 changes: 12 additions & 4 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ if (process.env.NODE_ENV !== 'development') {
Sentry.init({
dsn: SENTRY_DSN,
integrations: [Sentry.electronMinidumpIntegration()],

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return event
}
Comment on lines +72 to +75
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return null
},
})
}

Expand Down Expand Up @@ -208,7 +216,7 @@ app.whenReady().then(
appSettings = await getSettings()
nativeTheme.themeSource = appSettings.appearance.theme

await sendReport(appSettings.usageReport)
await sendReport(appSettings.telemetry.usageReport)
await createSplashWindow()
await setupProjectStructure()
await createWindow()
Expand Down Expand Up @@ -365,7 +373,7 @@ ipcMain.handle(
browserWindow,
resolvedScriptPath,
appSettings.proxy.port,
appSettings.usageReport.enabled
appSettings.telemetry.usageReport
)
}
)
Expand Down Expand Up @@ -648,8 +656,8 @@ async function applySettings(
if (modifiedSettings.recorder) {
appSettings.recorder = modifiedSettings.recorder
}
if (modifiedSettings.usageReport) {
appSettings.usageReport = modifiedSettings.usageReport
if (modifiedSettings.telemetry) {
appSettings.telemetry = modifiedSettings.telemetry
}
if (modifiedSettings.appearance) {
appSettings.appearance = modifiedSettings.appearance
Expand Down
38 changes: 37 additions & 1 deletion src/schemas/settings/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { describe, expect, it } from 'vitest'
import { migrate } from '.'
import * as v1 from './v1'
import * as v2 from './v2'

describe('Settings migration', () => {
it('should migrate from v1 to latest', () => {
Expand Down Expand Up @@ -29,6 +30,41 @@ describe('Settings migration', () => {
},
}

expect(migrate(v1Settings).version).toBe('2.0')
const migration = migrate(v1Settings)

expect(migration.version).toBe('3.0')
expect(migration.telemetry.usageReport).toBe(v1Settings.usageReport.enabled)
})

it('should migrate from v2 to latest', () => {
const v2Settings: v2.AppSettings = {
version: '2.0',
proxy: {
mode: 'regular',
port: 6000,
automaticallyFindPort: true,
},
recorder: {
detectBrowserPath: true,
},
windowState: {
width: 1200,
height: 800,
x: 0,
y: 0,
isMaximized: true,
},
usageReport: {
enabled: true,
},
appearance: {
theme: 'system',
},
}

const migration = migrate(v2Settings)

expect(migration.version).toBe('3.0')
expect(migration.telemetry.usageReport).toBe(v2Settings.usageReport.enabled)
})
})
8 changes: 6 additions & 2 deletions src/schemas/settings/index.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,22 @@
import { z } from 'zod'
import * as v1 from './v1'
import * as v2 from './v2'
import * as v3 from './v3'
import { exhaustive } from '../../utils/typescript'

const AnySettingSchema = z.discriminatedUnion('version', [
v1.AppSettingsSchema,
v2.AppSettingsSchema,
v3.AppSettingsSchema,
])

export function migrate(settings: z.infer<typeof AnySettingSchema>) {
switch (settings.version) {
case '1.0':
return migrate(v1.migrate(settings))
case '2.0':
return migrate(v2.migrate(settings))
case '3.0':
return settings
default:
return exhaustive(settings)
Expand All @@ -25,6 +29,6 @@ export {
AppearanceSchema,
ProxySettingsSchema,
RecorderSettingsSchema,
UsageReportSettingsSchema,
TelemetrySchema,
WindowStateSchema,
} from './v2'
} from './v3'
19 changes: 16 additions & 3 deletions src/schemas/settings/v2/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
UsageReportSettingsSchema,
WindowStateSchema,
} from '../v1'
import * as v3 from '../v3'

export {
AppearanceSchema,
Expand All @@ -26,7 +27,19 @@ export const AppSettingsSchema = z.object({

export type AppSettings = z.infer<typeof AppSettingsSchema>

// TODO: Migrate settings to the next version
export function migrate(settings: z.infer<typeof AppSettingsSchema>) {
return { ...settings }
// Migrate settings to the next version
export function migrate(
settings: z.infer<typeof AppSettingsSchema>
): v3.AppSettings {
return {
version: '3.0',
proxy: settings.proxy,
recorder: settings.recorder,
windowState: settings.windowState,
telemetry: {
usageReport: settings.usageReport.enabled,
errorReport: true,
},
appearance: settings.appearance,
}
}
36 changes: 36 additions & 0 deletions src/schemas/settings/v3/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { z } from 'zod'
import {
AppearanceSchema,
ProxySettingsSchema,
RecorderSettingsSchema,
WindowStateSchema,
} from '../v1'

const TelemetrySchema = z.object({
usageReport: z.boolean(),
errorReport: z.boolean(),
})

export {
AppearanceSchema,
ProxySettingsSchema,
RecorderSettingsSchema,
TelemetrySchema,
WindowStateSchema,
}

export const AppSettingsSchema = z.object({
version: z.literal('3.0'),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

proxy: ProxySettingsSchema,
recorder: RecorderSettingsSchema,
windowState: WindowStateSchema,
telemetry: TelemetrySchema,
appearance: AppearanceSchema,
})

export type AppSettings = z.infer<typeof AppSettingsSchema>

// TODO: Migrate settings to the next version
export function migrate(settings: z.infer<typeof AppSettingsSchema>) {
return { ...settings }
}
7 changes: 4 additions & 3 deletions src/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { safeJsonParse } from './utils/json'
import log from 'electron-log/main'

const defaultSettings: AppSettings = {
version: '2.0',
version: '3.0',
proxy: {
mode: 'regular',
port: 6000,
Expand All @@ -24,8 +24,9 @@ const defaultSettings: AppSettings = {
y: 0,
isMaximized: true,
},
usageReport: {
enabled: true,
telemetry: {
usageReport: true,
errorReport: true,
},
appearance: {
theme: 'system',
Expand Down
4 changes: 2 additions & 2 deletions src/types/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ import {
AppSettingsSchema,
ProxySettingsSchema,
RecorderSettingsSchema,
UsageReportSettingsSchema,
TelemetrySchema,
} from '@/schemas/settings'
import { z } from 'zod'

export type AppSettings = z.infer<typeof AppSettingsSchema>
export type ProxySettings = z.infer<typeof ProxySettingsSchema>
export type RecorderSettings = z.infer<typeof RecorderSettingsSchema>
export type UsageReportSettings = z.infer<typeof UsageReportSettingsSchema>
export type TelemetrySettings = z.infer<typeof TelemetrySchema>
5 changes: 2 additions & 3 deletions src/usageReport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { getPlatform, getArch } from './utils/electron'
import { writeFile, readFile } from 'fs/promises'
import path from 'path'
import { existsSync } from 'fs'
import { UsageReportSettings } from './types/settings'
import log from 'electron-log/main'

const URL = 'https://stats.grafana.org/k6-studio-usage-report'
Expand All @@ -20,8 +19,8 @@ export const getOrSetInstallationId = async () => {
return await readFile(filePath, { encoding: 'utf-8' })
}

export const sendReport = async (usageReportSettings: UsageReportSettings) => {
if (!usageReportSettings.enabled) {
export const sendReport = async (usageReportEnabled: boolean) => {
if (!usageReportEnabled) {
return
}

Expand Down
Loading