Improve extension loading state and starting

packages/nodejs/.changesets/change-the-way-the-extension-is-started.md

@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "change"
+---
+
+Change the way the extension is started. Previously it was started on initialization of the extension class (if the config was active), but now it's started by the client class.

packages/nodejs/.changesets/fix-extension-fallback-functions.md

@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "fix"
+---
+
+Fix the extension function fallbacks on installation failure. When the extension fails to install and calls are made to the not loaded functions, it will no longer throw an error.

packages/nodejs/.changesets/improve-extension-loading.md

@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "change"
+---
+
+Improve the reporting of extension loading state in the diagnose report. Previously it would only report it as loaded when it was also running.

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

@@ -1,3 +1,4 @@
+import { Extension } from "../extension"
 import { BaseClient } from "../client"
 import { BaseTracer as Tracer } from "../tracer"
 import { BaseMetrics as Metrics } from "../metrics"
@@ -19,13 +20,15 @@ describe("BaseClient", () => {
   })
 
   it("starts the client", () => {
+    const startSpy = jest.spyOn(client.extension, "start")
     client.start()
-    expect(client.isActive).toBeTruthy()
+    expect(startSpy).toHaveBeenCalled()
   })
 
   it("stops the client", () => {
+    const stopSpy = jest.spyOn(client.extension, "stop")
     client.stop()
-    expect(client.isActive).toBeFalsy()
+    expect(stopSpy).toHaveBeenCalled()
   })
 
   it("stores the client on global object", () => {
@@ -42,13 +45,15 @@ describe("BaseClient", () => {
 
   it("does not start the client if config is not valid", () => {
     process.env["APPSIGNAL_PUSH_API_KEY"] = undefined
+    const startSpy = jest.spyOn(client.extension, "start")
     client = new BaseClient({ name, enableMinutelyProbes: false })
-    expect(client.isActive).toBeFalsy()
+    expect(startSpy).not.toHaveBeenCalled()
   })
 
   it("starts the client when the active option is true", () => {
+    const startSpy = jest.spyOn(client.extension, "start")
     client = new BaseClient({ ...DEFAULT_OPTS, active: true })
-    expect(client.isActive).toBeTruthy()
+    expect(startSpy).not.toHaveBeenCalled()
   })
 
   it("returns a NoopTracer if the agent isn't started", () => {

packages/nodejs/src/__tests__/extension.failure.test.ts

@@ -0,0 +1,44 @@
+import { Extension } from "../extension"
+
+describe("Extension", () => {
+  let ext: Extension
+
+  beforeEach(() => {
+    ext = new Extension()
+  })
+
+  afterEach(() => {
+    ext.stop()
+  })
+
+  it("is not loaded", () => {
+    expect(Extension.isLoaded).toEqual(false)
+  })
+
+  it("does not start the client", () => {
+    const warnSpy = jest.spyOn(console, "warn")
+    expect(() => {
+      ext.start()
+    }).not.toThrow()
+    expect(warnSpy).toHaveBeenLastCalledWith(
+      "AppSignal extension not loaded. This could mean that your current environment isn't supported, or that another error has occurred."
+    )
+  })
+
+  it("does not error on stopping the client", () => {
+    expect(() => {
+      ext.stop()
+    }).not.toThrow()
+  })
+
+  it("does not error on diagnoseRaw", () => {
+    expect(ext.diagnose()).toMatchObject({
+      error: expect.any(Error),
+      output: [""]
+    })
+  })
+
+  it("does not error on runningInContainer", () => {
+    expect(ext.runningInContainer()).toBeUndefined()
+  })
+})

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

@@ -11,27 +11,19 @@ describe("Extension", () => {
     ext.stop()
   })
 
+  it("is loaded", () => {
+    expect(Extension.isLoaded).toEqual(true)
+  })
+
   it("starts the client", () => {
     expect(() => {
       ext.start()
     }).not.toThrow()
-
-    expect(ext.isLoaded).toBeTruthy()
   })
 
   it("stops the client", () => {
     expect(() => {
       ext.stop()
     }).not.toThrow()
-
-    expect(ext.isLoaded).toBeFalsy()
-  })
-
-  it("starts the client when the active option is true", () => {
-    expect(() => {
-      ext = new Extension({ active: true })
-    }).not.toThrow()
-
-    expect(ext.isLoaded).toBeTruthy()
   })
 })

packages/nodejs/src/client.ts

@@ -46,14 +46,11 @@ export class BaseClient implements Client {
     const { ignoreInstrumentation } = options
 
     this.config = new Configuration(options)
-
-    const active = this.config.data.active!
-
-    this.extension = new Extension({ active })
-
+    this.extension = new Extension()
     this.storeInGlobal()
 
     if (this.isActive) {
+      this.extension.start()
       this.#metrics = new BaseMetrics()
     } else {
       this.#metrics = new NoopMetrics()
@@ -69,7 +66,7 @@ export class BaseClient implements Client {
    * Returns `true` if the agent is loaded and configuration is valid
    */
   get isActive(): boolean {
-    return this.extension.isLoaded && this.config.isValid
+    return Extension.isLoaded && this.config.isValid && this.config.data.active!
   }
 
   set isActive(arg) {

packages/nodejs/src/diagnose.ts

@@ -39,7 +39,7 @@ export class DiagnoseTool {
 
   constructor() {
     this.#config = new Configuration({})
-    this.#extension = new Extension({ active: true })
+    this.#extension = new Extension()
   }
 
   /**
@@ -73,7 +73,7 @@ export class DiagnoseTool {
       language: "nodejs",
       package_version: VERSION,
       agent_version: AGENT_VERSION,
-      extension_loaded: this.#extension.isLoaded
+      extension_loaded: Extension.isLoaded
     }
   }
 

packages/nodejs/src/extension.ts

@@ -1,12 +1,12 @@
-import { extension } from "./extension_wrapper"
+import { extension, isLoaded as extensionLoaded } from "./extension_wrapper"
 
 /**
  * The public interface for the extension.
  *
  * @class
  */
 export class Extension {
-  isLoaded = false
+  static isLoaded = extensionLoaded
 
   constructor(options?: { active: boolean }) {
     if (options?.active) this.start()
@@ -15,10 +15,9 @@ export class Extension {
   /**
    * Starts the extension.
    */
-  public start(): boolean {
+  public start() {
     try {
       extension.start()
-      this.isLoaded = true
     } catch (e) {
       if (e.message === "Extension module not loaded") {
         console.warn(
@@ -29,41 +28,28 @@ export class Extension {
           `Failed to load AppSignal extension with error: ${e.message}. Please email us at [email protected] for support.`
         )
       }
-
-      this.isLoaded = false
     }
-
-    return this.isLoaded
   }
 
   /**
    * Stops the extension.
    */
-  public stop(): boolean {
-    if (this.isLoaded) {
-      extension.stop()
-      this.isLoaded = false
-    }
-
-    return this.isLoaded
+  public stop() {
+    extension.stop()
   }
 
   public diagnose(): object {
-    if (this.isLoaded) {
-      process.env._APPSIGNAL_DIAGNOSE = "true"
-      const diagnostics_report_string = extension.diagnoseRaw()
-      delete process.env._APPSIGNAL_DIAGNOSE
+    process.env._APPSIGNAL_DIAGNOSE = "true"
+    const diagnostics_report_string = extension.diagnoseRaw()
+    delete process.env._APPSIGNAL_DIAGNOSE
 
-      try {
-        return JSON.parse(diagnostics_report_string)
-      } catch (error) {
-        return {
-          error: error,
-          output: diagnostics_report_string.split("\n")
-        }
+    try {
+      return JSON.parse(diagnostics_report_string)
+    } catch (error) {
+      return {
+        error: error,
+        output: (diagnostics_report_string || "").split("\n")
       }
-    } else {
-      return {}
     }
   }
 

packages/nodejs/src/extension_wrapper.ts

@@ -4,15 +4,17 @@ let mod: ExtensionWrapper
 
 try {
   mod = require("../build/Release/extension.node") as ExtensionWrapper
+  mod.isLoaded = true
 } catch (e) {
   mod = {
+    isLoaded: false,
     extension: {
       start() {
         throw new Error("Extension module not loaded")
       },
-      stop() {
-        return
-      }
+      stop() {},
+      diagnoseRaw() {},
+      runningInContainer() {}
     }
   } as ExtensionWrapper
 }

packages/nodejs/src/interfaces/extension_wrapper.ts

@@ -1,4 +1,5 @@
 export interface ExtensionWrapper {
+  isLoaded: boolean
   extension: any
   span: any
   datamap: any