Skip to content

feat(node)!: split childProcess integration into childProcess and worker integrations - #22886

Open
atharv-sys32 wants to merge 1 commit into
getsentry:developfrom
atharv-sys32:feat/split-worker-integration
Open

feat(node)!: split childProcess integration into childProcess and worker integrations#22886
atharv-sys32 wants to merge 1 commit into
getsentry:developfrom
atharv-sys32:feat/split-worker-integration

Conversation

@atharv-sys32

@atharv-sys32 atharv-sys32 commented Jul 30, 2026

Copy link
Copy Markdown

Split worker thread handling out of childProcessIntegration into a new workerThreadsIntegration.

Previously childProcessIntegration captured both child process and worker thread events. Now:

  • childProcessIntegration() handles child process breadcrumbs only.
  • workerThreadsIntegration() (was workerIntegration) captures worker thread errors with an auto.worker_thread mechanism.

The deprecated captureWorkerErrors option is removed from both integrations per review feedback; worker thread errors are always captured.

Fixes #18698

@atharv-sys32
atharv-sys32 requested review from a team as code owners July 30, 2026 22:30
@atharv-sys32
atharv-sys32 requested review from isaacs and mydea and removed request for a team July 30, 2026 22:30
Comment thread packages/node/src/integrations/workerIntegration.ts Outdated
Comment thread packages/node/src/index.ts Outdated
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

👋 @isaacs, @mydea, @getsentry/team-javascript-sdks — Please review this PR when you get a chance!

@atharv-sys32
atharv-sys32 force-pushed the feat/split-worker-integration branch from bab9d05 to add0cf9 Compare August 6, 2026 10:13
Comment thread packages/node/src/integrations/childProcess.ts Outdated

@JPeer264 JPeer264 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It also seems that the build and the lint is failing. You can check out our CONTRIBUTING.md how to run yarn build and yarn lint

* @deprecated Use `workerIntegration({ captureWorkerErrors })` instead.
* This option is no longer used by childProcessIntegration.
*/
captureWorkerErrors?: boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: You can remove that entire option. Everything on develop can be breaking right now.

*
* @default true
*/
captureWorkerErrors?: boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: IMO we can remove the option entirely and make it only capturing errors. If they don't want to have it as an error they could opt-out of this integration. If there is a need again in the future for having that option, we could always re-add it. But less code is better here.

captureWorkerErrors?: boolean;
}

const INTEGRATION_NAME = 'Worker' as const;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That would make more sense IMO. Please also rename the integration (and file) to workerThreadsIntegration. Who knows what comes in the future that is a "worker" 😅

Suggested change
const INTEGRATION_NAME = 'Worker' as const;
const INTEGRATION_NAME = 'WorkerThreads' as const;

@atharv-sys32
atharv-sys32 force-pushed the feat/split-worker-integration branch from add0cf9 to 3267dd3 Compare August 6, 2026 12:56
Comment thread packages/node/src/index.ts
@atharv-sys32
atharv-sys32 force-pushed the feat/split-worker-integration branch from 3267dd3 to 886e88b Compare August 6, 2026 13:12
@atharv-sys32
atharv-sys32 requested a review from a team as a code owner August 6, 2026 13:12
@atharv-sys32
atharv-sys32 requested review from s1gr1d and removed request for a team August 6, 2026 13:12
@atharv-sys32

Copy link
Copy Markdown
Author

Hi @JPeer264, thanks for the review. I've addressed the feedback:

  • Removed the deprecated captureWorkerErrors option from childProcessIntegration, and removed captureWorkerErrors from the worker integration. Worker thread errors are now always captured.
  • Renamed workerIntegration to workerThreadsIntegration (file, function, and INTEGRATION_NAME = 'WorkerThreads'), with exports updated in node, aws-serverless, google-cloud-serverless, and astro.
  • Fixed the build failure: mechanism.data was passing undefined for threadId; it now omits the key when the thread id isn't available. Formatting is oxfmt-clean on all changed files.
  • Updated the process-thread e2e test to expect the worker error as an exception event instead of a breadcrumb.

CI is queued but the workflow needs maintainer approval to run. Could you approve it so the build and tests can validate?

@atharv-sys32
atharv-sys32 requested a review from JPeer264 August 6, 2026 13:30
@JPeer264 JPeer264 changed the title feat: split childProcess integration into childProcess and worker integrations feat(node)!: split childProcess integration into childProcess and worker integrations Aug 6, 2026
@atharv-sys32
atharv-sys32 force-pushed the feat/split-worker-integration branch from 886e88b to 0c0a9b0 Compare August 6, 2026 15:04
Comment thread packages/node/src/integrations/workerThreadsIntegration.ts
@atharv-sys32
atharv-sys32 force-pushed the feat/split-worker-integration branch from 0c0a9b0 to 32eb52d Compare August 6, 2026 15:14
@atharv-sys32

atharv-sys32 commented Aug 6, 2026

Copy link
Copy Markdown
Author

Apologies for the back and forth on this, the failures were on my end. The issues should all be resolved now:

  • Fixed the failing integration tests. A test was still expecting the old auto.child_process.worker_thread mechanism type; updated to auto.worker_thread.
  • Rebased onto the latest develop so the branch is up to date.
  • Updated MIGRATION.md for the workerThreadsIntegration rename, and noted that captureWorkerErrors is removed and that disabling childProcessIntegration no longer disables worker error capture.

All the review feedback is addressed (option removal, rename, astro export, tests, docs). Could you approve the workflow again so we can get the remaining checks green and proceed @JPeer264 ?

@JPeer264
JPeer264 requested review from andreiborza and removed request for isaacs, mydea and s1gr1d August 7, 2026 06:49

@JPeer264 JPeer264 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Amazing thanks a lot for the contribution. LGTM

Waiting for one more review just in case I missed something

@atharv-sys32
atharv-sys32 force-pushed the feat/split-worker-integration branch from 32eb52d to b8fbdf6 Compare August 7, 2026 07:30
Comment thread packages/node/src/index.ts
Comment thread dev-packages/node-integration-tests/suites/breadcrumbs/process-thread/app.mjs Outdated
@atharv-sys32
atharv-sys32 force-pushed the feat/split-worker-integration branch 2 times, most recently from 32eb52d to 326f5a2 Compare August 7, 2026 08:22
@atharv-sys32

Copy link
Copy Markdown
Author

Amazing thanks a lot for the contribution. LGTM

Waiting for one more review just in case I missed something

Thanks @JPeer264 much appreciated , i noticed the pipeline ran around 40 mins ago failed - one Node 26 integration test : suites/breadcrumbs/process-thread (ESM). It expects the worker thread error event to arrive before the thrown test error, but on Node 26 the order occasionally flips as it doesn't happen locally with me. Do we have to do something about it ?

Also as the branch was outdated , I have rebased with the latest dev

@atharv-sys32
atharv-sys32 force-pushed the feat/split-worker-integration branch from 326f5a2 to fae8cfd Compare August 7, 2026 08:44
Comment thread packages/node/src/integrations/workerThreadsIntegration.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit fae8cfd. Configure here.

…ads integrations

Splits worker thread handling out of childProcessIntegration into a new
workerThreadsIntegration. The deprecated captureWorkerErrors option is
removed from both integrations; worker thread errors are now always
captured. Exports the new integration from the astro server barrel and
updates the e2e tests to expect worker thread error events with the
auto.worker_thread mechanism. Uses unordered event matching in the
process-thread e2e test since the two captured events can arrive in
either order. Updates MIGRATION.md for the rename.

Fixes getsentry#18698

Signed-off-by: Atharv Pandey <atharvpandey245@gmail.com>
@atharv-sys32
atharv-sys32 force-pushed the feat/split-worker-integration branch from fae8cfd to 61d2126 Compare August 7, 2026 09:22
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.

Split up childProcess integration into childProcess and worker integration

2 participants