Skip to content

Commit

Permalink
fix(cli): Prevent unwanted SIGINT error message from storybook (#8977)
Browse files Browse the repository at this point in the history
**Problem**
Storybook is showing an error message when you exit via a SIGINT. 

We have spent a few hours trying to understand why and work around this
with no success.

*Changes**
1. Conditionally disable the additional default signal listeners based
on the argv contents.
2. Emit a custom event on `process` to trigger the telemetry shutdown.

**Notes**
I'm not really proud of this PR. Disabling based on the contents of argv
isn't all that robust against false positives. Emitting a custom event
on `process` is not really supposed to be done - hence why TS is
complaining about the name of the signal.

This work has also highlighted that on my windows 11 machine the
background scripts are not firing... This was working when I updated the
span values to match those found to work in this PR comment
nodejs/node#21825 (comment) when
I reenabled windows telemetry. Maybe something has changed, maybe it's
version specific (os or node). This is an active nodejs issue so my
ability to solve this in redwood code is understandably limited.

Why isn't our CI finding this? We should investigate this too.

@jtoar Could you confirm on mac and windows that this PR prevents those
additional error messages from storybook. If you also have the time I'd
appreciate if you could check if the telemetry background job is firing
when you test on windows.

I'll test this on ubuntu locally.
  • Loading branch information
Josh-Walker-GM authored and jtoar committed Jul 27, 2023
1 parent 2d15787 commit 0cfc508
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 6 deletions.
2 changes: 2 additions & 0 deletions packages/cli-packages/storybook/src/commands/storybook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ export async function handler(options: StorybookYargsOptions): Promise<void> {
open: options.open,
smokeTest: options.smokeTest,
})
// @ts-expect-error - Custom workaround for storybook telemetry
process.emit('shutdown-telemetry')

const { handler: storybookHandler } = await import('./storybookHandler.js')
await storybookHandler(options)
Expand Down
20 changes: 14 additions & 6 deletions packages/cli/src/telemetry/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
SimpleSpanProcessor,
SamplingDecision,
} from '@opentelemetry/sdk-trace-node'
import { hideBin } from 'yargs/helpers'

import { spawnBackgroundProcess } from '../lib/background'

Expand Down Expand Up @@ -72,12 +73,19 @@ export async function startTelemetry() {
// then we leave it to that handler to handle the signal.
// See https://nodejs.org/dist/latest/docs/api/process.html#signal-events for more info on the
// behaviour of nodejs for various signals.
for (const signal of ['SIGTERM', 'SIGINT', 'SIGHUP']) {
process.on(signal, () => {
if (process.listenerCount(signal) === 1) {
console.log(`Received ${signal} signal, exiting...`)
process.exit()
}
const cleanArgv = hideBin(process.argv)
if (!cleanArgv.includes('sb') && !cleanArgv.includes('storybook')) {
for (const signal of ['SIGTERM', 'SIGINT', 'SIGHUP']) {
process.on(signal, () => {
if (process.listenerCount(signal) === 1) {
console.log(`Received ${signal} signal, exiting...`)
process.exit()
}
})
}
} else {
process.on('shutdown-telemetry', () => {
shutdownTelemetry()
})
}

Expand Down

0 comments on commit 0cfc508

Please sign in to comment.