Skip to content

Telemetry: Improve dev cancellation handling #32218

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

Merged
merged 1 commit into from
Aug 8, 2025

Conversation

shilman
Copy link
Member

@shilman shilman commented Aug 8, 2025

Closes N/A

What I did

Fix for #32168. The previous PR works when you run the process directly in node and cancel out with the keyboard. However, when you run through an outer process like nr storybook it does not get the SIGINT signal. This attempts to fix that case.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  1. Install the canary in a project
  2. Run it with STORYBOOK_TELEMETRY_DEBUG=1 nr storybook
  3. Kill it with ctrl-C and verify that the canceled event is sent.

⚠️ NOTE: this does not fix the problem. But it hopefully fixes other related issues.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-32218-sha-525e5323. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information
Published version 0.0.0-pr-32218-sha-525e5323
Triggered by @shilman
Repository storybookjs/storybook
Branch shilman/fix-cancel-telemetry
Commit 525e5323
Datetime Fri Aug 8 02:07:25 UTC 2025 (1754618845)
Workflow run 16820177616

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=32218

Greptile Summary

This PR adds SIGTERM signal handling to improve telemetry cancellation when Storybook is terminated through wrapper processes. The change is minimal but important - it adds a single line in code/core/src/core-server/dev-server.ts that registers the existing cancelTelemetry function to handle SIGTERM signals alongside the existing SIGINT handler.

The core issue being addressed is that when Storybook runs through process managers or wrapper commands like nr storybook, the SIGINT signal (typically sent by Ctrl-C) doesn't always propagate properly to the Node.js process. SIGTERM is the standard termination signal used by most process managers and container orchestrators for graceful shutdowns. By adding SIGTERM handling, the PR ensures telemetry data is properly sent even when Storybook is terminated through these wrapper processes.

This change integrates seamlessly with the existing signal handling architecture in Storybook's development server. The implementation follows the same pattern as the SIGINT handler, maintaining consistency in the codebase and ensuring that the same telemetry cleanup process runs regardless of which termination signal is received.

Confidence score: 5/5

  • This PR is extremely safe to merge with virtually no risk of causing issues
  • Score reflects the minimal, well-understood change that follows existing patterns and addresses a legitimate operational concern
  • No files require special attention - the single line addition is straightforward and follows established conventions

@shilman shilman added bug patch:yes Bugfix & documentation PR that need to be picked to main branch telemetry labels Aug 8, 2025
Copy link

nx-cloud bot commented Aug 8, 2025

View your CI Pipeline Execution ↗ for commit 525e532

Command Status Duration Result
nx run-many -t check -c production --parallel=7 ✅ Succeeded 1m 2s View ↗
nx run-many -t build -c production --parallel=3 ✅ Succeeded 4m 26s View ↗

☁️ Nx Cloud last updated this comment at 2025-08-08 06:38:23 UTC

@storybook-pr-benchmarking
Copy link

Package Benchmarks

Commit: 525e532, ran on 8 August 2025 at 06:43:11 UTC

The following packages have significant changes to their size or dependencies:

@storybook/builder-webpack5

Before After Difference
Dependency count 198 198 0
Self size 80 KB 80 KB 0 B
Dependency size 31.37 MB 31.38 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

storybook

Before After Difference
Dependency count 51 51 0
Self size 41.74 MB 41.75 MB 🚨 +15 KB 🚨
Dependency size 18.13 MB 18.13 MB 0 B
Bundle Size Analyzer Link Link

@storybook/angular

Before After Difference
Dependency count 198 198 0
Self size 192 KB 191 KB 🎉 -445 B 🎉
Dependency size 29.57 MB 29.59 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

@storybook/ember

Before After Difference
Dependency count 203 203 0
Self size 28 KB 28 KB 0 B
Dependency size 28.10 MB 28.11 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

@storybook/nextjs

Before After Difference
Dependency count 538 538 0
Self size 903 KB 903 KB 0 B
Dependency size 59.12 MB 59.13 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

@storybook/nextjs-vite

Before After Difference
Dependency count 130 130 0
Self size 3.08 MB 3.08 MB 0 B
Dependency size 22.28 MB 22.29 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

@storybook/react-native-web-vite

Before After Difference
Dependency count 163 163 0
Self size 32 KB 32 KB 0 B
Dependency size 23.66 MB 23.68 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

@storybook/react-vite

Before After Difference
Dependency count 120 120 0
Self size 32 KB 32 KB 0 B
Dependency size 20.23 MB 20.24 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

@storybook/react-webpack5

Before After Difference
Dependency count 286 286 0
Self size 25 KB 25 KB 0 B
Dependency size 43.69 MB 43.70 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

@storybook/server-webpack5

Before After Difference
Dependency count 210 210 0
Self size 17 KB 17 KB 0 B
Dependency size 32.64 MB 32.65 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

sb

Before After Difference
Dependency count 52 52 0
Self size 1 KB 1 KB 0 B
Dependency size 59.87 MB 59.89 MB 🚨 +15 KB 🚨
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 220 220 0
Self size 585 KB 585 KB 🎉 -18 B 🎉
Dependency size 104.36 MB 104.39 MB 🚨 +29 KB 🚨
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 189 189 0
Self size 31 KB 31 KB 0 B
Dependency size 88.44 MB 88.47 MB 🚨 +29 KB 🚨
Bundle Size Analyzer Link Link

@storybook/preset-react-webpack

Before After Difference
Dependency count 177 177 0
Self size 24 KB 24 KB 0 B
Dependency size 30.51 MB 30.53 MB 🚨 +14 KB 🚨
Bundle Size Analyzer Link Link

@shilman shilman changed the title Telemetry: Fix dev cancellation handling Telemetry: Improve dev cancellation handling Aug 8, 2025
@shilman shilman marked this pull request as ready for review August 8, 2025 14:53
@shilman shilman merged commit a65cd6b into next Aug 8, 2025
63 of 68 checks passed
@shilman shilman deleted the shilman/fix-cancel-telemetry branch August 8, 2025 14:53
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:normal patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch telemetry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants