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: Additional context and breadcrumbs #390

Merged
merged 10 commits into from
Dec 6, 2021

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Dec 1, 2021

This PR:

  • Refactors and simplifies src/main/context.ts a little
    • Adds context.app.app_start_time
  • Renames ElectronEvents integration to ElectronBreadcrumbs. We should use Sentry terminology to avoid confusion.
    • Makes all the breadcrumb categories optional and enabled by default
    • Allows fine-grained configuration over which events need breadcrumbs
    • Adds breadcrumbs for BrowserWindow and autoUpdater events
    • For BrowserWindow and WebContents events, we now include the id and window title as extra breadcrumb data
  • Adds default enabled AdditionalContext integration with all options enabled by default { cpu, screen, memory, language }
    • cpu adds processor_count, cpu_description and processor_frequency to context.device
      • Adds context.device.machine_arch = 'arm64' if machine arch is arm64 and running in emulation
    • screen adds screen_resolution and screen_density to context.device
    • memory adds memory_size and free_memory to context.device
    • language adds language to context.device
  • Updates the test events and test normalization to test much of the above

All context in src/main/context.ts is not user configurable. The AdditionalContext integration allow this extra context to be configurable. The only downside to using an integration is that anyone using the ElectronMinidump uploader rather than the default sentry uploader will not get this additional context because those events don't go through the event processors.

Closes #386

ElectronBreadcrumbs configuration

Stop getting app events:

new ElectronBreadcrumbs({ app: false })

Only get show and hide events for BrowserWindow

new ElectronBreadcrumbs({ browserWindow: ['show', 'hide'] })

Get all app events apart from those starting with remote-:

new ElectronBreadcrumbs({ app: (name) => !name.startsWith('remote-') })

@timfish timfish marked this pull request as ready for review December 2, 2021 13:17
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

1st pass - exciting!!

src/main/context.ts Outdated Show resolved Hide resolved
src/main/context.ts Outdated Show resolved Hide resolved
src/main/context.ts Outdated Show resolved Hide resolved
src/main/integrations/additional-context.ts Outdated Show resolved Hide resolved
src/main/integrations/additional-context.ts Outdated Show resolved Hide resolved
src/main/integrations/electron-breadcrumbs.ts Outdated Show resolved Hide resolved
src/main/integrations/electron-breadcrumbs.ts Outdated Show resolved Hide resolved
src/main/integrations/electron-breadcrumbs.ts Outdated Show resolved Hide resolved
src/main/integrations/electron-breadcrumbs.ts Outdated Show resolved Hide resolved
test/e2e/recipe/normalize.ts Show resolved Hide resolved
}

return await defaultsPromise;
return await cachedDefaultsPromise;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should cache the actual value instead of having to resolve the promise every time.

src/main/integrations/electron-breadcrumbs.ts Outdated Show resolved Hide resolved
app_version: app.getVersion(),
},
app: getAppContext() as Record<string, any>,
os: (await getOsContext()) as Record<string, any>,
Copy link
Member

Choose a reason for hiding this comment

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

Not ideal that we have to type cast here, but we can live with it

test/e2e/recipe/normalize.ts Show resolved Hide resolved
src/main/integrations/electron-breadcrumbs.ts Show resolved Hide resolved
@timfish timfish merged commit 11a6d8c into getsentry:master Dec 6, 2021
@timfish timfish deleted the feat/additional-context branch December 8, 2021 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instrument BrowserWindow events
2 participants