chore(cli-storybook): Add telemetry

[Josh-Walker-GM]

Instruments the storybook command with telemetry. I had forgot about it since it lives in it's own package now.

WIP
I had to add an empty SIGINT listener for execa to exit and the process.on exit to trigger. I'll need to understand why...

[jtoar]

@Josh-Walker-GM I know we tested it locally, but I'm not sure that registering on process.on('exit') is enough for SIGINT; the Node.js docs say...

The 'exit' event is emitted when the Node.js process is about to exit as a result of either:

• The process.exit() method being called explicitly;
• The Node.js event loop no longer having any additional work to perform.

Source: https://nodejs.org/dist/latest-v18.x/docs/api/process.html#event-exit.

We could call process.exit in process.on('SIGINT')'s handler. But I also don't like calling process.exit explicitly if we don't hae to. I know it was somewhat necessary for deeply nested functions, but I'm not sure if that's the case here yet

[jtoar]

Just curious why we record attributes twice?

[Josh-Walker-GM]

I had to record the command name inside the middleware because it might not have been recorded in the handler is if the check had throw an error.

We probably don't need the command: 'storybook' in the handler recordTelemetryAttributes since it'll be set by the middleware. I kept it there mainly to just be consistent that all handlers set the command name.

[Josh-Walker-GM]

@Josh-Walker-GM I know we tested it locally, but I'm not sure that registering on process.on('exit') is enough for SIGINT; the Node.js docs say...

The 'exit' event is emitted when the Node.js process is about to exit as a result of either:

• The process.exit() method being called explicitly;
• The Node.js event loop no longer having any additional work to perform.

Source: https://nodejs.org/dist/latest-v18.x/docs/api/process.html#event-exit.

We could call process.exit in process.on('SIGINT')'s handler. But I also don't like calling process.exit explicitly if we don't hae to. I know it was somewhat necessary for deeply nested functions, but I'm not sure if that's the case here yet

Thanks @jtoar this was definitely something I was coming back to since this PR highlighted the weird SIGINT behaviour.

I've read over more of the node docs and this was particularly helpful:

'SIGTERM' and 'SIGINT' have default handlers on non-Windows platforms that reset the terminal mode before exiting with code 128 + signal number. If one of these signals has a listener installed, its default behavior will be removed (Node.js will no longer exit).

I tested to ensure that the default exit causes by SIGINT does not trigger the process exit event - it does not.

Taking a step back and looking at our requirements might be useful. We must have the telemetry shutdown fire before the process exits. This is achieved by having it called on the process exit event. We know now that we must ensure node's other mechanisms of termination trigger the process exit event.

It would seem to me then that the best course of action would be to provide listeners on the SIGINT and SIGTERM which call process.exit. However, I agree that this is not optimal since we may have circumstances where we don't want SIGINT to immediately call process.exit. My suggestion then would be to have these default listeners for SIGINT/SIGTERM only call process.exit when no other listeners are registered on those events. Something like:

process.on('SIGINT', () => {
  if(process.listenerCount('SIGINT') > 1){
    return
  }
  process.exit()
})

That means we're free to have custom behaviour for these events where we need them. If we do this then I can't think of a way in which node exits without a call to process.exit or just gracefully when it's got no other work left.

There are still some signals where we don't have the power to intervene on like SIGKILL.

Lastly, also from the docs:

'SIGHUP' is generated on Windows when the console window is closed, and on other platforms under various similar conditions. See signal(7). It can have a listener installed, however Node.js will be unconditionally terminated by Windows about 10 seconds later. On non-Windows platforms, the default behavior of SIGHUP is to terminate Node.js, but once a listener has been installed its default behavior will be removed.

We'll probably want to include this default exit behaviour for SIGHUP too. Making sure we exit within the roughly 10s grace period windows would give us - likely not a problem.

---

EDIT: Pushed up what I was talking about for an example.

[Josh-Walker-GM]

Okay I think this is ready now @jtoar but I'll take another look over this with fresh eyes tomorrow to check.

[Tobbe]

I haven't tested this code, but just reading it it looks good to me