Fix enableMinutelyProbes behaviour

Before this change, setting `enableMinutelyProbes` to `false`
would not disable the minutely probes system, it merely does not
register the default minutely probe.

The flag's behaviour has been changed so that the minutely probes
system is disabled in its entirety when it is set to `false`. The
`Probes` object within `Metrics` is replaced by a `NoopProbes`
object, which does nothing when a probe is registered.

packages/nodejs/.changesets/fix-enableminutelyprobes-behaviour.md

@@ -0,0 +1,21 @@
+---
+bump: "patch"
+type: "fix"
+---
+
+Setting `enableMinutelyProbes` to `false` now disables the minutely probes
+system. Custom probes will not be called when the minutely probes are
+disabled. In addition, the `APPSIGNAL_ENABLE_MINUTELY_PROBES` environment
+variable can now be used to enable or disable the minutely probes.
+
+Before this change, setting `enableMinutelyProbes` to `false` would not
+register the default Node.js heap statistics minutely probe, but custom
+probes would still be called. To opt in for the previous behaviour,
+disabling only the Node.js heap statistics minutely probe without disabling
+custom probes, you can use the `probes.unregister()` method to unregister
+the default probe:
+
+```js
+const probes = appsignal.metrics().probes();
+probes.unregister("v8_stats");
+```

packages/nodejs/src/__tests__/bootstrap.test.ts

@@ -41,13 +41,8 @@ describe("Bootstrap", () => {
     })
 
     it("bootstraps the core probes", () => {
-      initCoreProbes(mock, { enableMinutelyProbes: true })
+      initCoreProbes(mock)
       expect(registerMock).toHaveBeenCalledTimes(1)
     })
-
-    it("doesn't bootstrap the core probes if enableMinutelyProbes is false", () => {
-      initCoreProbes(mock, { enableMinutelyProbes: false })
-      expect(registerMock).toHaveBeenCalledTimes(0)
-    })
   })
 })

packages/nodejs/src/__tests__/metrics.test.ts

@@ -1,12 +1,26 @@
+import { BaseClient } from "../client"
 import { BaseMetrics as Metrics } from "../metrics"
+import { NoopProbes } from "../noops"
+import { BaseProbes } from "../probes"
 
 describe("Metrics", () => {
   let metrics: Metrics
 
   beforeEach(() => {
+    new BaseClient()
     metrics = new Metrics()
   })
 
+  it("has `Probes` when minutely probes are on", () => {
+    expect(metrics.probes()).toBeInstanceOf(BaseProbes)
+  })
+
+  it("has `NoopProbes` when minutely probes are off", () => {
+    new BaseClient({ enableMinutelyProbes: false })
+    metrics = new Metrics()
+    expect(metrics.probes()).toBeInstanceOf(NoopProbes)
+  })
+
   it("sets a gauge", () => {
     expect(() => {
       metrics.setGauge("database_size", 100)

packages/nodejs/src/__tests__/probes.test.ts

@@ -1,5 +1,6 @@
-import { Metrics } from "../interfaces"
-import { BaseProbes as Probes } from "../probes"
+import { Metrics, Probes } from "../interfaces"
+import { BaseProbes } from "../probes"
+import { BaseMetrics } from "../metrics"
 import * as v8 from "../probes/v8"
 import os from "os"
 import { BaseClient } from "../client"
@@ -9,30 +10,61 @@ describe("Probes", () => {
 
   beforeEach(() => {
     jest.useFakeTimers()
-    probes = new Probes()
+    probes = new BaseProbes()
   })
 
   afterEach(() => {
     jest.clearAllTimers()
   })
 
-  it("registers a probe", () => {
+  function registerMockProbe(): jest.Mock {
     const fn = jest.fn()
     probes.register("test_metric", fn)
+    return fn
+  }
+
+  it("registers a probe", () => {
+    const fn = registerMockProbe()
     jest.runOnlyPendingTimers()
     expect(fn).toHaveBeenCalled()
     expect(probes.count).toEqual(1)
   })
 
   it("unregisters a probe", () => {
-    const fn = jest.fn()
-    probes.register("test_metric", fn)
+    const fn = registerMockProbe()
     probes.unregister("test_metric")
     jest.runOnlyPendingTimers()
     expect(fn).not.toHaveBeenCalled()
     expect(probes.count).toEqual(0)
   })
 
+  describe("Metrics integration test", () => {
+    function initialiseMetrics(enableMinutelyProbes: boolean = true) {
+      const client = new BaseClient({
+        active: true,
+        pushApiKey: "TEST_API_KEY",
+        enableMinutelyProbes
+      })
+      expect(client.metrics()).toBeInstanceOf(BaseMetrics)
+
+      probes = client.metrics().probes()
+    }
+
+    it("calls probes when enableMinutelyProbes is true", () => {
+      initialiseMetrics(true)
+      const fn = registerMockProbe()
+      jest.runOnlyPendingTimers()
+      expect(fn).toHaveBeenCalled()
+    })
+
+    it("does not call probes when enableMinutelyProbes is false", () => {
+      initialiseMetrics(false)
+      const fn = registerMockProbe()
+      jest.runOnlyPendingTimers()
+      expect(fn).not.toHaveBeenCalled()
+    })
+  })
+
   describe("v8 probe", () => {
     let meterMock: Metrics
     let setGaugeMock: jest.Mock
@@ -46,7 +78,7 @@ describe("Probes", () => {
     })
 
     function registerV8Probe(hostname?: string) {
-      new BaseClient({ hostname: hostname })
+      new BaseClient({ hostname })
 
       let { PROBE_NAME, init } = v8
       probes.register(PROBE_NAME, init(meterMock))

packages/nodejs/src/bootstrap.ts

@@ -3,7 +3,6 @@ import { Instrumentation } from "./instrument"
 import { httpPlugin, httpsPlugin } from "./instrumentation/http"
 import * as pgPlugin from "./instrumentation/pg"
 import * as redisPlugin from "./instrumentation/redis"
-
 import * as gcProbe from "./probes/v8"
 
 /**
@@ -39,16 +38,9 @@ export function initCorePlugins(
 /**
  * Initialises all the available probes to attach automatically at runtime.
  */
-export function initCoreProbes(
-  meter: Metrics,
-  { enableMinutelyProbes }: { enableMinutelyProbes?: boolean }
-) {
+export function initCoreProbes(meter: Metrics) {
   let probes: any[] = [gcProbe]
 
-  if (!enableMinutelyProbes) {
-    return
-  }
-
   // load probes
   probes.forEach(({ PROBE_NAME, init }) =>
     meter.probes().register(PROBE_NAME, init(meter))

packages/nodejs/src/client.ts

@@ -26,7 +26,7 @@ export class BaseClient implements Client {
   instrumentation: Instrumentation
 
   #tracer: Tracer = new BaseTracer()
-  #metrics: Metrics = new BaseMetrics()
+  #metrics: Metrics
 
   /**
    * Global accessors to Client and Config
@@ -45,19 +45,24 @@ export class BaseClient implements Client {
   constructor(options: Partial<AppsignalOptions> = {}) {
     const {
       active = false, // Agent is not started by default
-      ignoreInstrumentation,
-      enableMinutelyProbes = true
+      ignoreInstrumentation
     } = options
 
     this.config = new Configuration(options)
     this.extension = new Extension({ active })
 
+    this.storeInGlobal()
+
+    if (this.isActive) {
+      this.#metrics = new BaseMetrics()
+    } else {
+      this.#metrics = new NoopMetrics()
+    }
+
     this.instrumentation = new Instrumentation(this.tracer(), this.metrics())
 
     initCorePlugins(this.instrumentation, { ignoreInstrumentation })
-    initCoreProbes(this.metrics(), { enableMinutelyProbes })
-
-    this.storeInGlobal()
+    initCoreProbes(this.metrics())
   }
 
   /**
@@ -129,10 +134,6 @@ export class BaseClient implements Client {
    * to easily spot the differences between contexts.
    */
   public metrics(): Metrics {
-    if (!this.isActive) {
-      return new NoopMetrics()
-    }
-
     return this.#metrics
   }
 

packages/nodejs/src/metrics.ts

@@ -1,15 +1,27 @@
 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"
 
 /**
  * The metrics object.
  *
  * @class
  */
 export class BaseMetrics implements Metrics {
-  #probes = new BaseProbes()
+  #probes: Probes
+
+  constructor() {
+    let enableMinutelyProbes = BaseClient.config.data.enableMinutelyProbes
+
+    if (enableMinutelyProbes) {
+      this.#probes = new BaseProbes()
+    } else {
+      this.#probes = new NoopProbes()
+    }
+  }
 
   /**
    * A gauge is a metric value at a specific time. If you set more

packages/nodejs/src/noops/index.ts

@@ -1,3 +1,4 @@
 export * from "./span"
 export * from "./tracer"
 export * from "./metrics"
+export * from "./probes"

packages/nodejs/src/noops/metrics.ts

@@ -1,8 +1,8 @@
 import { Metrics, Probes } from "../interfaces"
-import { BaseProbes } from "../probes"
+import { NoopProbes } from "../noops"
 
 export class NoopMetrics implements Metrics {
-  #probes = new BaseProbes()
+  #probes = new NoopProbes()
 
   public setGauge(
     key: string,

packages/nodejs/src/noops/probes.ts

@@ -0,0 +1,19 @@
+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
+  }
+}