Only report extension load errors on start

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 `this.#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.
appsignal · May 10, 2024 · 6bf596c · 6bf596c
1 parent c975d9d
commit 6bf596c
Show file tree

Hide file tree

Showing 8 changed files with 123 additions and 87 deletions.
diff --git a/.changesets/improve-error-reporting-during-initialisation.md b/.changesets/improve-error-reporting-during-initialisation.md
@@ -0,0 +1,10 @@
+---
+bump: "patch"
+type: "change"
+---
+
+### Improve error reporting during initialisation
+
+Do not report an error with the extension installation when AppSignal is imported -- instead, report it when attempting to initialise AppSignal. Do not report an error with the extension if AppSignal is not configured to be active.
+
+When AppSignal does not start due to its configuration (`active` is set to `false`, or the push API key is missing) report the specific reason why.
diff --git a/src/__tests__/client.test.ts b/src/__tests__/client.test.ts
@@ -27,19 +27,19 @@ describe("Client", () => {
     await client.stop()
   })
 
-  it("starts the client", () => {
+  it("starts the extension when the client is active", () => {
     const startSpy = jest.spyOn(Extension.prototype, "start")
-    client.start()
+    client = new Client({ ...DEFAULT_OPTS, active: true })
     expect(startSpy).toHaveBeenCalled()
   })
 
-  it("stops the client", async () => {
+  it("stops the extension when the client is stopped", async () => {
     const extensionStopSpy = jest.spyOn(Extension.prototype, "stop")
     await client.stop()
     expect(extensionStopSpy).toHaveBeenCalled()
   })
 
-  it("stops the probes when the client is active", async () => {
+  it("starts and stops the probes when the client is active", async () => {
     client = new Client({ ...DEFAULT_OPTS, active: true })
     const probes = client.metrics().probes()
     expect(probes.isRunning).toEqual(true)

diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts
@@ -477,25 +477,30 @@ describe("Configuration", () => {
     })
   })
 
-  describe(".isValid", () => {
-    it("is valid if pushApiKey is present", () => {
-      config = new Configuration({ pushApiKey })
-      expect(config.isValid).toBeTruthy()
+  describe(".validationError", () => {
+    it("is valid if pushApiKey is present and active is not false", () => {
+      config = new Configuration({ pushApiKey: "something", active: true })
+      expect(config.validationError()).toBeFalsy()
     })
 
     it("is invalid if pushApiKey is not present", () => {
-      config = new Configuration({ name })
-      expect(config.isValid).toBeFalsy()
+      config = new Configuration({ active: true })
+      expect(config.validationError()).toEqual("Push API key is not present")
     })
 
     it("is invalid if pushApiKey is an empty string", () => {
-      config = new Configuration({ name, pushApiKey: "" })
-      expect(config.isValid).toBeFalsy()
+      config = new Configuration({ pushApiKey: "", active: true })
+      expect(config.validationError()).toEqual("Push API key is not present")
     })
 
     it("is invalid if pushApiKey is a string with only whitespaces", () => {
-      config = new Configuration({ name, pushApiKey: "  " })
-      expect(config.isValid).toBeFalsy()
+      config = new Configuration({ pushApiKey: "  ", active: true })
+      expect(config.validationError()).toEqual("Push API key is not present")
+    })
+
+    it("is invalid if active is false", () => {
+      config = new Configuration({ pushApiKey: "something", active: false })
+      expect(config.validationError()).toEqual("AppSignal is not active")
     })
   })
 })
diff --git a/src/__tests__/extension.failure.test.ts b/src/__tests__/extension.failure.test.ts
@@ -1,28 +1,38 @@
 import fs from "fs"
 import { Extension } from "../extension"
+import { Client } from "../client"
 import {
   reportPath,
   processTarget
 } from "../../scripts/extension/support/helpers"
 
+const CLIENT_OPTIONS = {
+  name: "app",
+  pushApiKey: "yes",
+  active: true
+}
+
 describe("Extension", () => {
   let ext: Extension
+  let client: Client | undefined
 
   beforeEach(() => {
     ext = new Extension()
+    client = undefined
   })
 
   afterEach(() => {
     ext.stop()
+    if (client) client.stop()
   })
 
-  it("logs an error when the module is required", () => {
+  it("logs an error when an active client is initialised", () => {
     const errorSpy = jest.spyOn(console, "error")
 
     jest.resetModules()
-    require("../extension")
+    client = new Client(CLIENT_OPTIONS)
 
-    expect(errorSpy).toHaveBeenLastCalledWith(
+    expect(errorSpy).toHaveBeenCalledWith(
       "[appsignal][ERROR] AppSignal failed to load the extension. Please run the diagnose tool and email us at [email protected]: https://docs.appsignal.com/nodejs/3.x/command-line/diagnose.html\n",
       expect.any(Object)
     )
@@ -56,9 +66,9 @@ describe("Extension", () => {
       const arch = process.arch
 
       jest.resetModules()
-      require("../extension")
+      client = new Client(CLIENT_OPTIONS)
 
-      expect(errorSpy).toHaveBeenLastCalledWith(
+      expect(errorSpy).toHaveBeenCalledWith(
         `[appsignal][ERROR] The AppSignal extension was installed for architecture 'dummyArch-dummyTarget', but the current architecture is '${arch}-${target}'. Please reinstall the AppSignal package on the host the app is started.`
       )
     })
@@ -82,13 +92,13 @@ describe("Extension", () => {
       const errorSpy = jest.spyOn(console, "error")
 
       jest.resetModules()
-      require("../extension")
+      client = new Client(CLIENT_OPTIONS)
 
       expect(errorSpy).toHaveBeenCalledWith(
         "[appsignal][ERROR] Unable to fetch install report:",
         expect.any(Object)
       )
-      expect(errorSpy).toHaveBeenLastCalledWith(
+      expect(errorSpy).toHaveBeenCalledWith(
         "[appsignal][ERROR] AppSignal failed to load the extension. Please run the diagnose tool and email us at [email protected]: https://docs.appsignal.com/nodejs/3.x/command-line/diagnose.html\n",
         expect.any(Object)
       )

diff --git a/src/client.ts b/src/client.ts
@@ -100,6 +100,7 @@ export class Client {
   #metrics: Metrics
   #sdk?: NodeSDK
   #additionalInstrumentations: NodeSDKInstrumentationsOption
+  #isActive: boolean
 
   /**
    * Global accessors for the AppSignal client
@@ -142,7 +143,9 @@ export class Client {
   }
 
   /**
-   * Creates a new instance of the `Appsignal` object
+   * Starts AppSignal with the given configuration. If no configuration is set
+   * yet it will try to automatically load the configuration using the
+   * environment loaded from environment variables.
    */
   constructor(options: Partial<Options> = {}) {
     this.#additionalInstrumentations = options.additionalInstrumentations || []
@@ -152,55 +155,49 @@ export class Client {
     this.internalLogger = this.setupInternalLogger()
     this.storeInGlobal()
 
-    if (this.isActive) {
-      if (process.env._APPSIGNAL_DIAGNOSE === "true") {
-        Client.internalLogger.info(
-          "Diagnose mode is enabled, not starting extension, SDK and probes"
-        )
-        this.#metrics = new NoopMetrics()
-      } else {
-        this.start()
-        this.#metrics = new Metrics()
-        if (this.config.data.initializeOpentelemetrySdk) {
-          this.#sdk = this.initOpenTelemetry()
-          this.setUpOpenTelemetryLogger()
-        }
-      }
-    } else {
-      this.#metrics = new NoopMetrics()
-      console.error("AppSignal not starting, no valid configuration found")
+    // These will be overwritten if AppSignal can be started
+    this.#metrics = new NoopMetrics()
+    this.#isActive = false
+
+    if (process.env._APPSIGNAL_DIAGNOSE === "true") {
+      this.internalLogger.info(
+        "AppSignal not starting: running in diagnose mode"
+      )
+
+      return
     }
 
-    this.initCoreProbes()
-  }
+    const validationError = this.config.validationError()
 
-  /**
-   * Returns `true` if the extension is loaded and configuration is valid
-   */
-  get isActive(): boolean {
-    return (
-      Extension.isLoaded &&
-      this.config.isValid &&
-      (this.config.data.active ?? false)
-    )
-  }
+    if (validationError) {
+      console.info(`AppSignal not starting: ${validationError}`)
 
-  set isActive(arg) {
-    console.warn("Cannot set isActive property")
-  }
+      return
+    }
 
-  /**
-   * Starts AppSignal with the given configuration. If no configuration is set
-   * yet it will try to automatically load the configuration using the
-   * environment loaded from environment variables and the current working
-   * directory.
-   */
-  public start(): void {
-    if (this.config.isValid) {
-      this.extension.start()
-    } else {
-      console.error("Not starting, no valid AppSignal configuration found")
+    if (!Extension.isLoaded) {
+      this.extension.logLoadingErrors()
+
+      console.error("AppSignal not starting: extension failed to load")
+
+      return
     }
+
+    this.extension.start()
+
+    this.#metrics = new Metrics()
+    this.#isActive = true
+
+    if (this.config.data.initializeOpentelemetrySdk) {
+      this.#sdk = this.initOpenTelemetry()
+      this.setUpOpenTelemetryLogger()
+    }
+
+    this.initCoreProbes()
+  }
+
+  public get isActive(): boolean {
+    return this.#isActive
   }
 
   /**

diff --git a/src/config.ts b/src/config.ts
@@ -43,10 +43,19 @@ export class Configuration {
   }
 
   /**
-   * Returns `true` if the current configuration is valid.
+   * Returns a (truthy) validation error message string if the current
+   * configuration is invalid, or `false` if the configuration is valid.
    */
-  public get isValid(): boolean {
-    return (this.data.pushApiKey || "").trim() !== ""
+  public validationError(): string | false {
+    if (this.data.active === false) {
+      return "AppSignal is not active"
+    }
+
+    if ((this.data.pushApiKey || "").trim() === "") {
+      return "Push API key is not present"
+    }
+
+    return false
   }
 
   public get logFilePath(): string | undefined {

diff --git a/src/extension.ts b/src/extension.ts
@@ -86,4 +86,8 @@ export class Extension {
   public runningInContainer(): boolean {
     return extension.runningInContainer()
   }
+
+  public logLoadingErrors() {
+    extension.logLoadingErrors()
+  }
 }
diff --git a/src/extension_wrapper.ts b/src/extension_wrapper.ts
@@ -27,25 +27,6 @@ try {
   mod = require("../build/Release/extension.node") as ExtensionWrapper
   mod.isLoaded = true
 } catch (error) {
-  const [installArch, installTarget] = fetchInstalledArch()
-  const arch = process.arch
-  const target = processTarget()
-
-  if (
-    installArch &&
-    installTarget &&
-    (arch !== installArch || target !== installTarget)
-  ) {
-    console.error(
-      `[appsignal][ERROR] The AppSignal extension was installed for architecture '${installArch}-${installTarget}', but the current architecture is '${arch}-${target}'. Please reinstall the AppSignal package on the host the app is started.`
-    )
-  } else {
-    console.error(
-      "[appsignal][ERROR] AppSignal failed to load the extension. Please run the diagnose tool and email us at [email protected]: https://docs.appsignal.com/nodejs/3.x/command-line/diagnose.html\n",
-      error
-    )
-  }
-
   mod = {
     isLoaded: false,
     extension: {
@@ -66,6 +47,26 @@ try {
       },
       log() {
         return
+      },
+      logLoadingErrors() {
+        const [installArch, installTarget] = fetchInstalledArch()
+        const arch = process.arch
+        const target = processTarget()
+
+        if (
+          installArch &&
+          installTarget &&
+          (arch !== installArch || target !== installTarget)
+        ) {
+          console.error(
+            `[appsignal][ERROR] The AppSignal extension was installed for architecture '${installArch}-${installTarget}', but the current architecture is '${arch}-${target}'. Please reinstall the AppSignal package on the host the app is started.`
+          )
+        } else {
+          console.error(
+            "[appsignal][ERROR] AppSignal failed to load the extension. Please run the diagnose tool and email us at [email protected]: https://docs.appsignal.com/nodejs/3.x/command-line/diagnose.html\n",
+            error
+          )
+        }
       }
     }
   } as ExtensionWrapper