Skip to content
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

Appsignal is not stopping correctly #418

Closed
janhalama opened this issue Jul 30, 2021 · 7 comments · Fixed by #612
Closed

Appsignal is not stopping correctly #418

janhalama opened this issue Jul 30, 2021 · 7 comments · Fixed by #612
Labels

Comments

@janhalama
Copy link

Hi there,
Appsignal is not stopping correctly which is causing jest tests to hang. You can reproduce this issue in following repository https://github.com/janhalama/appsignal-test by running yarn test command.

This is screenshot of the running test:
image

I tried to find answer in existing issues, found this one related #398 but answers did not help me much.

@ErfanEbrahimnia
Copy link

I'm experiencing the same issue. Did you find a solution @janhalama?

@janhalama
Copy link
Author

No i did not find solution.

@tombruijn
Copy link
Member

Hi there, do you want to use AppSignal in the test environment? If not, I recommend disabling AppSignal in the test env like described on this page: https://docs.appsignal.com/nodejs/configuration/#disable-appsignal-for-tests

Alternatively you could call the stop function on the AppSignal object in a global callback after the test suite is done?

@freaz
Copy link

freaz commented Aug 9, 2021

Thanks for reaching back.

Skipping AppSignal is possible, but not desired as we want run e2e test including AppSignal.

And calling stop in global callback (Jest's afterAll) doesn't work either.

@ErfanEbrahimnia
Copy link

Basically the issue is that the stop method isn't doing anything when called

@tombruijn
Copy link
Member

I had a quick look. I ran it with jest --detectOpenHandles and it told me the problem was in the minutely probe process that was still running.

If I add this config to the example linked above, it will not enable the minutely probes and work around this issue. The appsignal.stop line can also be removed then.

diff --git app.js app.js
index 037c00a..228ea8e 100644
--- app.js
+++ app.js
@@ -7,6 +7,7 @@ function startApp() {
     active: true,
     name: "express-test",
     apiKey: process.env.APPSIGNAL_PUSH_API_KEY,
+    enableMinutelyProbes: false
   });

I hope that works for everyone for now. We'll discuss this issue with the team to figure out the best way to shut down the minutely probes process on Appsignal.stop or when the Node process stops.

unflxw added a commit that referenced this issue Feb 11, 2022
Implement a `.stop` method on the `BaseProbes` class and on the
`Probes` interface, along with a read-only `.isStopped` property.

The new `BaseProbes` object handles only the stopping logic. The
actual behaviour of the probes is delegated to two private classes:
`StartedProbes` (which used to be `BaseProbes`) and `StoppedProbes`
(which used to be `NoopProbes`). Both of these implement the
`InternalProbes` private interface. Stopping the probes permanently
switches the implementation used.

`NoopProbes` no longer exists, but `BaseProbes({ stopped: true })`
can be used to initialise it in the stopped state, which is
functionally equivalent.

Call `metrics().probes().stop()` from the `BaseClient.stop()`
method, ensuring the probes system is stopped when AppSignal is
stopped.

Fixes #418 and closes #569.
unflxw added a commit that referenced this issue Feb 11, 2022
Implement a `.stop` method on the `BaseProbes` class and on the
`Probes` interface, along with a read-only `.isRunning` property.

The new `BaseProbes` object handles only the stopping logic. The
actual behaviour of the probes is delegated to two private classes:
`BaseProbeRunner` (which used to be `BaseProbes`) and
`NoopProbeRunner` (which used to be `NoopProbes`). Both of these
implement the `ProbeRunner` private interface. Stopping the probes
permanently switches the implementation used.

`NoopProbes` no longer exists, but `BaseProbes({ run: false })`
can be used to initialise it in the stopped state, which is
functionally equivalent.

Call `metrics().probes().stop()` from the `BaseClient.stop()`
method, ensuring the probes system is stopped when AppSignal is
stopped.

Fixes #418 and closes #569.
unflxw added a commit that referenced this issue Feb 11, 2022
Implement a `.stop` method on the `BaseProbes` class and on the
`Probes` interface, along with a read-only `.isRunning` property.

The new `BaseProbes` object handles only the stopping logic. The
actual behaviour of the probes is delegated to two private classes:
`BaseProbeRunner` (which used to be `BaseProbes`) and
`NoopProbeRunner` (which used to be `NoopProbes`). Both of these
implement the `ProbeRunner` private interface. Stopping the probes
permanently switches the implementation used.

`NoopProbes` no longer exists, but `BaseProbes({ run: false })`
can be used to initialise it in the stopped state, which is
functionally equivalent.

Call `metrics().probes().stop()` from the `BaseClient.stop()`
method, ensuring the probes system is stopped when AppSignal is
stopped.

Fixes #418 and closes #569.
unflxw added a commit that referenced this issue Feb 11, 2022
Implement a `.stop` method on the `BaseProbes` class and on the
`Probes` interface, along with a read-only `.isRunning` property.

The new `BaseProbes` object handles only the stopping logic. The
actual behaviour of the probes is delegated to two private classes:
`BaseProbeRunner` (which used to be `BaseProbes`) and
`NoopProbeRunner` (which used to be `NoopProbes`). Both of these
implement the `ProbeRunner` private interface. Stopping the probes
permanently switches the implementation used.

`NoopProbes` no longer exists, but `BaseProbes({ run: false })`
can be used to initialise it in the stopped state, which is
functionally equivalent.

Call `metrics().probes().stop()` from the `BaseClient.stop()`
method, ensuring the probes system is stopped when AppSignal is
stopped.

Fixes #418 and closes #569.
unflxw added a commit that referenced this issue Feb 14, 2022
Implement a `.stop` method on the `BaseProbes` class and on the
`Probes` interface, along with a read-only `.isRunning` property.

The new `BaseProbes` object handles only the stopping logic. The
actual behaviour of the probes is delegated to two private classes:
`BaseProbeRunner` (which used to be `BaseProbes`) and
`NoopProbeRunner` (which used to be `NoopProbes`). Both of these
implement the `ProbeRunner` private interface. Stopping the probes
permanently switches the implementation used.

`NoopProbes` no longer exists, but `BaseProbes({ run: false })`
can be used to initialise it in the stopped state, which is
functionally equivalent.

Call `metrics().probes().stop()` from the `BaseClient.stop()`
method, ensuring the probes system is stopped when AppSignal is
stopped.

Fixes #418 and closes #569.
@unflxw
Copy link
Contributor

unflxw commented Feb 15, 2022

Hi @janhalama @ErfanEbrahimnia @freaz! This issue should be fixed in version 2.3.2 of the @appsignal/nodejs package. Calling appsignal.stop() after your tests (i.e. in an afterAll() block, or in Jest's globalTeardown config property) should clear all pending minutely probes, avoiding the issue where the process keeps running after the tests are done.

Please let us know if you run into any further issues with the package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants