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(node): Rework ANR to use worker script via an integration #9823

Merged
merged 30 commits into from
Dec 19, 2023

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Dec 13, 2023

This PR reworks Node ANR detection to use a worker thread. Workers are usually started via a path to a source file but this can cause issues when bundlers are used. Instead, the worker code is bundled and included in the source as a base64 string which can be used to launch a worker via a data URL. The base64 code comes in at around 45KB.

Closes #9324 by adding trace context.

Also aids with solving getsentry/sentry-electron#784

Relies on #9018 being merged so module and app/device context are included.

Positives 👍

  • No extra processes
  • Only 10-15MB memory overhead (vs at least 50MB for a child process)
  • Uses inspector API so we can remove the websockets implementation
  • Doesn't require special cases/setup for Electron main process
  • No longer runs the app entry point again as the child process code
    • Closes some outstanding Electron ANR issues
    • ANR becomes just an integration since we don't need to intercept app execution in the child
    • Less confusing setup and less chance of running the app twice

Negatives 👎

  • Minimum supported Node version for ANR detection becomes v16 because Node 14 does not support data URLs for workers 😢

Usage

import * as Sentry from '@sentry/node';

Sentry.init({
  dsn: 'https://public@dsn.ingest.sentry.io/1337',
  integrations: [new Sentry.Integrations.Anr({ captureStackTrace: true, anrThreshold: 200 })],
});

@timfish timfish marked this pull request as ready for review December 13, 2023 20:56
@timfish

This comment was marked as outdated.

packages/node-integration-tests/suites/anr/test.ts Outdated Show resolved Hide resolved
packages/node/src/integrations/anr/index.ts Show resolved Hide resolved
packages/node/src/integrations/anr/index.ts Outdated Show resolved Hide resolved
packages/node/src/integrations/anr/index.ts Outdated Show resolved Hide resolved
packages/node/src/integrations/anr/worker.ts Outdated Show resolved Hide resolved
packages/node/src/integrations/anr/worker.ts Show resolved Hide resolved
packages/node/src/integrations/anr/worker.ts Outdated Show resolved Hide resolved
packages/node/src/integrations/anr/worker.ts Show resolved Hide resolved

const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout);

parentPort?.on('message', (msg: { session: Session | undefined }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parentPort?.on('message', (msg: { session: Session | undefined }) => {
parentPort?.on('message', (msg: { session?: Session }) => {

rollup/bundleHelpers.js Show resolved Hide resolved
packages/node/src/integrations/anr/worker.ts Outdated Show resolved Hide resolved
packages/node/src/integrations/anr/worker.ts Show resolved Hide resolved
packages/node/src/integrations/anr/index.ts Show resolved Hide resolved
@timfish
Copy link
Collaborator Author

timfish commented Dec 19, 2023

This PR now also fetches the parentSpanId and I added some test asserts that check the app/device/os/culture context gets added to events.

@AbhiPrasad AbhiPrasad merged commit ce9efc7 into getsentry:develop Dec 19, 2023
75 checks passed
@timfish timfish deleted the anr-worker-from-base64-script branch December 20, 2023 04:17
@lforst
Copy link
Member

lforst commented Dec 20, 2023

Emitting files into our src folder is very bad for a lot of reasons - caching, build order, general reproducability. We need to change this.

@lforst
Copy link
Member

lforst commented Dec 20, 2023

Pls do not do this immediately as I am restructuring our rollup configs right now and I am hotfixing this but this is high prio.

@lforst
Copy link
Member

lforst commented Dec 20, 2023

Sorry I have to revert this PR. It has too many bad implications with the build process.

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.

Node ANR tracking not getting trace context attached
4 participants