appsignal / appsignal-nodejs

🟩 AppSignal for Node.js
https://www.appsignal.com/nodejs/
MIT License
28 stars 9 forks source link

Only report extension load errors on start #1051

Closed unflxw closed 4 months ago

unflxw commented 4 months ago

Before this change, extension load errors would be reported whenever @appsignal/nodejs or the client.ts file was imported. Now they will only be reported whenever Client fails to initialise.

This fixes an issue where the extension load errors would be reported when running npm install in the appsignal-nodejs folder.

Rearrange the way that the client is initialised. Remove the client.start() method, as starting is done automatically during initialisation. Use early returns to clearly distinguish the client initialisation failure scenarios. Emit distinct error messages for different configuration-related failures.

Change the Client.isActive method to reflect whether the client was actually initialised, rather than whether several factors in the configuration should mean that the client was initialised.

This is an improvement to https://github.com/appsignal/appsignal-nodejs/issues/960, as AppSignal will no longer show an error about the extension not loading when active is set to false. It would still be nice to have a dedicated, non-scary Windows error.

tombruijn commented 4 months ago

I'm okay with this change but you need to update the tests for this.

unflxw commented 4 months ago

@tombruijn Yeah, the tests are correct to be failing -- now the message is not emitted at all. I'll see what I can do to fix it.

backlog-helper[bot] commented 4 months ago

:heavy_check_mark: All good!

New issue guide | Backlog management | Rules | Feedback

unflxw commented 4 months ago

@tombruijn It's been fixed now -- but I've also changed a lot of other stuff in the process. I couldn't understand what the right place to emit these warnings in the client initialisation was, so I ended up rearranging it significantly to figure it out.

backlog-helper[bot] commented 4 months ago

This is a message from the daily scheduled checks.

New issue guide | Backlog management | Rules | Feedback

tombruijn commented 4 months ago

Worth it to add a changeset .