Improve extension loading state and starting

## The problem

The `Extension`'s '`isLoaded` attribute was only set when the
extension's `start` function was called. This made it difficult to
determine if the extension was really loaded in the app or not, without
starting the extension first.

This made it necessary to start the extension in scenarios where we need
to call extension functions without needing to starting it first, such
as in the diagnose command.

Calling `Extension`'s `start` doesn't change any state in the extension
that needs to be set before any other function can be called. I think it
was written this way as a way to communicate loading issues. A better
name for this attribute would have been `isRunning`, but this is not
something we have to track.

## Solution

Update how the `isLoaded` is set, by checking if the extension was
loaded into the app or not on load, not just when `start` is called.

I've chosen to communicate this with an export from the extension
wrapper, rather than if the start function returns an error or not.
Since the value doesn't change at runtime it's now a static attribute of
the class.

I've removed the `isLoaded` return values from the extension's `start`
and `stop` function, these are unused.

## About the error thrown by `start`

I've left the `Extension`'s `start` function intact to print an error
when called and the extension isn't loaded. In other integrations we
print this on load of the extension. I suspect this was done on start,
so that it wouldn't always be printed on unsupported Operating Systems.
We may want to revisit this, or find another way to not print this all
the time for those systems.

## Only start extension when its loaded and active

Do not automatically start the extension when its initialized, but do
this from the client like in all the other integrations. This also makes
it easier to initialize the `Extension` class for other uses, like the
diagnose without having to worry about the active state or not.

Closes #551

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/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,33 @@
+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()
+  })
+})

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,8 +4,10 @@ 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")

packages/nodejs/src/interfaces/extension_wrapper.ts

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