From 58a597efb5956f5c000a4be05e9fb86bfdf4c90f Mon Sep 17 00:00:00 2001 From: Noemi Date: Fri, 11 Feb 2022 14:03:58 +0100 Subject: [PATCH] Implement stop method for probes 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. --- ...minutely-probes-when-stopping-appsignal.md | 6 ++ packages/nodejs/src/__tests__/client.test.ts | 20 ++++-- packages/nodejs/src/__tests__/metrics.test.ts | 10 ++- packages/nodejs/src/__tests__/probes.test.ts | 24 +++++++ packages/nodejs/src/client.ts | 1 + .../http/lifecycle/__tests__/outgoing.test.ts | 8 ++- packages/nodejs/src/interfaces/probes.ts | 11 +++ packages/nodejs/src/metrics.ts | 7 +- packages/nodejs/src/noops/index.ts | 1 - packages/nodejs/src/noops/metrics.ts | 4 +- packages/nodejs/src/noops/probes.ts | 19 ------ packages/nodejs/src/probes/index.ts | 67 ++++++++++++++++--- 12 files changed, 129 insertions(+), 49 deletions(-) create mode 100644 packages/nodejs/.changesets/stop-minutely-probes-when-stopping-appsignal.md delete mode 100644 packages/nodejs/src/noops/probes.ts diff --git a/packages/nodejs/.changesets/stop-minutely-probes-when-stopping-appsignal.md b/packages/nodejs/.changesets/stop-minutely-probes-when-stopping-appsignal.md new file mode 100644 index 000000000..a05e45ca3 --- /dev/null +++ b/packages/nodejs/.changesets/stop-minutely-probes-when-stopping-appsignal.md @@ -0,0 +1,6 @@ +--- +bump: "patch" +type: "fix" +--- + +The minutely probes are now stopped when `Appsignal.stop()` is called. This fixes an issue where Jest tests would warn about asynchronous operations that remain pending after the tests. diff --git a/packages/nodejs/src/__tests__/client.test.ts b/packages/nodejs/src/__tests__/client.test.ts index b2745cc8e..f1040134a 100644 --- a/packages/nodejs/src/__tests__/client.test.ts +++ b/packages/nodejs/src/__tests__/client.test.ts @@ -12,13 +12,16 @@ describe("BaseClient", () => { let client: BaseClient - // enableMinutelyProbes is set to false so we don't leak timers - const DEFAULT_OPTS = { name, pushApiKey, enableMinutelyProbes: false } + const DEFAULT_OPTS = { name, pushApiKey } beforeEach(() => { client = new BaseClient({ ...DEFAULT_OPTS }) }) + afterEach(() => { + client.stop() + }) + it("starts the client", () => { const startSpy = jest.spyOn(client.extension, "start") client.start() @@ -26,9 +29,18 @@ describe("BaseClient", () => { }) it("stops the client", () => { - const stopSpy = jest.spyOn(client.extension, "stop") + const extensionStopSpy = jest.spyOn(client.extension, "stop") + client.stop() + expect(extensionStopSpy).toHaveBeenCalled() + }) + + it("stops the probes when the client is active", () => { + client = new BaseClient({ ...DEFAULT_OPTS, active: true }) + const probes = client.metrics().probes() + expect(probes.isRunning).toEqual(true) + client.stop() - expect(stopSpy).toHaveBeenCalled() + expect(probes.isRunning).toEqual(false) }) it("stores the client on global object", () => { diff --git a/packages/nodejs/src/__tests__/metrics.test.ts b/packages/nodejs/src/__tests__/metrics.test.ts index 7306375cc..10f8d4087 100644 --- a/packages/nodejs/src/__tests__/metrics.test.ts +++ b/packages/nodejs/src/__tests__/metrics.test.ts @@ -1,7 +1,5 @@ import { BaseClient } from "../client" import { BaseMetrics as Metrics } from "../metrics" -import { NoopProbes } from "../noops" -import { BaseProbes } from "../probes" describe("Metrics", () => { let metrics: Metrics @@ -11,14 +9,14 @@ describe("Metrics", () => { metrics = new Metrics() }) - it("has `Probes` when minutely probes are on", () => { - expect(metrics.probes()).toBeInstanceOf(BaseProbes) + it("runs the probes when minutely probes are on", () => { + expect(metrics.probes().isRunning).toEqual(true) }) - it("has `NoopProbes` when minutely probes are off", () => { + it("does not run the probes when minutely probes are off", () => { new BaseClient({ enableMinutelyProbes: false }) metrics = new Metrics() - expect(metrics.probes()).toBeInstanceOf(NoopProbes) + expect(metrics.probes().isRunning).toEqual(false) }) it("sets a gauge", () => { diff --git a/packages/nodejs/src/__tests__/probes.test.ts b/packages/nodejs/src/__tests__/probes.test.ts index 3a77df7c1..91a0cb4d6 100644 --- a/packages/nodejs/src/__tests__/probes.test.ts +++ b/packages/nodejs/src/__tests__/probes.test.ts @@ -23,6 +23,30 @@ describe("Probes", () => { return fn } + describe("when stopped", () => { + it("is not running", () => { + expect(probes.isRunning).toEqual(true) + probes.stop() + expect(probes.isRunning).toEqual(false) + }) + + it("does not register or call an already registered probe", () => { + const fn = registerMockProbe() + probes.stop() + jest.runOnlyPendingTimers() + expect(fn).not.toHaveBeenCalled() + expect(probes.count).toEqual(0) + }) + + it("does not register or call a newly registered probe", () => { + probes.stop() + const fn = registerMockProbe() + jest.runOnlyPendingTimers() + expect(fn).not.toHaveBeenCalled() + expect(probes.count).toEqual(0) + }) + }) + it("registers a probe", () => { const fn = registerMockProbe() jest.runOnlyPendingTimers() diff --git a/packages/nodejs/src/client.ts b/packages/nodejs/src/client.ts index 6ba55fd6e..91b8ea82e 100644 --- a/packages/nodejs/src/client.ts +++ b/packages/nodejs/src/client.ts @@ -105,6 +105,7 @@ export class BaseClient implements Client { console.log("Stopping AppSignal") } + this.metrics().probes().stop() this.extension.stop() } diff --git a/packages/nodejs/src/instrumentation/http/lifecycle/__tests__/outgoing.test.ts b/packages/nodejs/src/instrumentation/http/lifecycle/__tests__/outgoing.test.ts index e8a0186c5..43fe475fa 100644 --- a/packages/nodejs/src/instrumentation/http/lifecycle/__tests__/outgoing.test.ts +++ b/packages/nodejs/src/instrumentation/http/lifecycle/__tests__/outgoing.test.ts @@ -14,12 +14,10 @@ describe("HTTP outgoing requests", () => { let client: Client let tracer: Tracer - // enableMinutelyProbes is set to false so we don't leak timers const DEFAULT_OPTS = { active: true, name, - pushApiKey, - enableMinutelyProbes: false + pushApiKey } beforeEach(() => { @@ -30,6 +28,10 @@ describe("HTTP outgoing requests", () => { instrument(http, tracer).install() }) + afterEach(() => { + client.stop() + }) + async function performRequest() { return new Promise>((resolve, reject) => { const options = { diff --git a/packages/nodejs/src/interfaces/probes.ts b/packages/nodejs/src/interfaces/probes.ts index 2eaecd600..880c8d6e1 100644 --- a/packages/nodejs/src/interfaces/probes.ts +++ b/packages/nodejs/src/interfaces/probes.ts @@ -2,6 +2,17 @@ * The Minutely probes object. */ export interface Probes { + /** + * Permanently stops the probes system, unregistering all probes + * and clearing the timers. + */ + stop(): this + + /** + * Whether the probes system is running. + */ + readonly isRunning: boolean + /** * Number of probes that are registered. */ diff --git a/packages/nodejs/src/metrics.ts b/packages/nodejs/src/metrics.ts index f9df6ad36..14b269b45 100644 --- a/packages/nodejs/src/metrics.ts +++ b/packages/nodejs/src/metrics.ts @@ -1,6 +1,5 @@ import { Metrics, Probes } from "./interfaces" import { BaseProbes } from "./probes" -import { NoopProbes } from "./noops" import { metrics } from "./extension_wrapper" import { Data } from "./internal/data" import { BaseClient } from "./client" @@ -16,11 +15,7 @@ export class BaseMetrics implements Metrics { constructor() { let enableMinutelyProbes = BaseClient.config.data.enableMinutelyProbes - if (enableMinutelyProbes) { - this.#probes = new BaseProbes() - } else { - this.#probes = new NoopProbes() - } + this.#probes = new BaseProbes({ run: enableMinutelyProbes }) } /** diff --git a/packages/nodejs/src/noops/index.ts b/packages/nodejs/src/noops/index.ts index c1e744c1e..46c856fd7 100644 --- a/packages/nodejs/src/noops/index.ts +++ b/packages/nodejs/src/noops/index.ts @@ -1,4 +1,3 @@ export * from "./span" export * from "./tracer" export * from "./metrics" -export * from "./probes" diff --git a/packages/nodejs/src/noops/metrics.ts b/packages/nodejs/src/noops/metrics.ts index 70f9863ae..8db53fed1 100644 --- a/packages/nodejs/src/noops/metrics.ts +++ b/packages/nodejs/src/noops/metrics.ts @@ -1,8 +1,8 @@ import { Metrics, Probes } from "../interfaces" -import { NoopProbes } from "../noops" +import { BaseProbes } from "../probes" export class NoopMetrics implements Metrics { - #probes = new NoopProbes() + #probes = new BaseProbes({ run: false }) public setGauge( key: string, diff --git a/packages/nodejs/src/noops/probes.ts b/packages/nodejs/src/noops/probes.ts deleted file mode 100644 index f01ba7148..000000000 --- a/packages/nodejs/src/noops/probes.ts +++ /dev/null @@ -1,19 +0,0 @@ -import { Probes } from "../interfaces" - -export class NoopProbes implements Probes { - public get count(): number { - return 0 - } - - public register(name: string, fn: () => void): this { - return this - } - - public unregister(name: string): this { - return this - } - - public clear(): this { - return this - } -} diff --git a/packages/nodejs/src/probes/index.ts b/packages/nodejs/src/probes/index.ts index 18d9adfe6..80abd5301 100644 --- a/packages/nodejs/src/probes/index.ts +++ b/packages/nodejs/src/probes/index.ts @@ -4,7 +4,54 @@ import { Probes } from "../interfaces" /** * The Minutely probes object. */ -export class BaseProbes extends EventEmitter implements Probes { +export class BaseProbes implements Probes { + #probes: ProbeRunner + #running = true + + constructor({ run = true } = {}) { + this.#probes = new BaseProbeRunner() + if (!run) this.stop() + } + + public stop(): this { + this.#probes.clear() + this.#probes = new NoopProbeRunner() + this.#running = false + return this + } + + get isRunning(): boolean { + return this.#running + } + + get count(): number { + return this.#probes.count + } + + public register(name: string, fn: () => void): this { + this.#probes.register(name, fn) + return this + } + + public unregister(name: string): this { + this.#probes.unregister(name) + return this + } + + public clear(): this { + this.#probes.clear() + return this + } +} + +type ProbeRunner = { + readonly count: number + register(name: string, fn: () => void): void + unregister(name: string): void + clear(): void +} + +class BaseProbeRunner extends EventEmitter implements ProbeRunner { #timers = new Map() constructor() { @@ -22,17 +69,17 @@ export class BaseProbes extends EventEmitter implements Probes { * Registers a new minutely probe. Using a probe `name` that has already been set * will overwrite the current probe. */ - public register(name: string, fn: () => void): this { + public register(name: string, fn: () => void): void { this.#timers.set( name, setInterval(() => this.emit(name), 60 * 1000) ) this.removeAllListeners(name) - return this.on(name, fn) + this.on(name, fn) } - public unregister(name: string): this { + public unregister(name: string): void { const timer = this.#timers.get(name) if (typeof timer !== "undefined") { @@ -40,17 +87,21 @@ export class BaseProbes extends EventEmitter implements Probes { this.#timers.delete(name) this.removeAllListeners(name) } - - return this } /** * Unregisters all probes and clears the timers. */ - public clear(): this { + public clear(): void { this.#timers.forEach(t => clearInterval(t)) this.#timers = new Map() this.removeAllListeners() - return this } } + +class NoopProbeRunner implements ProbeRunner { + readonly count: number = 0 + public register(_name: string, _fn: () => void): void {} + public unregister(_name: string): void {} + public clear(): void {} +}