diff --git a/packages/nodejs/.changesets/change-the-way-the-extension-is-started.md b/packages/nodejs/.changesets/change-the-way-the-extension-is-started.md new file mode 100644 index 000000000..6776eab45 --- /dev/null +++ b/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. diff --git a/packages/nodejs/.changesets/improve-extension-loading.md b/packages/nodejs/.changesets/improve-extension-loading.md new file mode 100644 index 000000000..3d5ecadff --- /dev/null +++ b/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. diff --git a/packages/nodejs/src/__tests__/client.test.ts b/packages/nodejs/src/__tests__/client.test.ts index 1bcf75380..b2745cc8e 100644 --- a/packages/nodejs/src/__tests__/client.test.ts +++ b/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", () => { diff --git a/packages/nodejs/src/__tests__/extension.failure.test.ts b/packages/nodejs/src/__tests__/extension.failure.test.ts new file mode 100644 index 000000000..b4738e630 --- /dev/null +++ b/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() + }) +}) diff --git a/packages/nodejs/src/__tests__/extension.test.ts b/packages/nodejs/src/__tests__/extension.test.ts index 46c98f2ad..f4dda180d 100644 --- a/packages/nodejs/src/__tests__/extension.test.ts +++ b/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() }) }) diff --git a/packages/nodejs/src/client.ts b/packages/nodejs/src/client.ts index 342b436b3..fd4dbea69 100644 --- a/packages/nodejs/src/client.ts +++ b/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) { diff --git a/packages/nodejs/src/diagnose.ts b/packages/nodejs/src/diagnose.ts index 149839cef..c096184ca 100644 --- a/packages/nodejs/src/diagnose.ts +++ b/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 } } diff --git a/packages/nodejs/src/extension.ts b/packages/nodejs/src/extension.ts index bb495451e..bf12b2612 100644 --- a/packages/nodejs/src/extension.ts +++ b/packages/nodejs/src/extension.ts @@ -1,4 +1,4 @@ -import { extension } from "./extension_wrapper" +import { extension, isLoaded as extensionLoaded } from "./extension_wrapper" /** * The public interface for the extension. @@ -6,7 +6,7 @@ import { extension } from "./extension_wrapper" * @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 support@appsignal.com 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 {} } } diff --git a/packages/nodejs/src/extension_wrapper.ts b/packages/nodejs/src/extension_wrapper.ts index 622cd2c8f..c2510d974 100644 --- a/packages/nodejs/src/extension_wrapper.ts +++ b/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") diff --git a/packages/nodejs/src/interfaces/extension_wrapper.ts b/packages/nodejs/src/interfaces/extension_wrapper.ts index 29601586e..3daa899cf 100644 --- a/packages/nodejs/src/interfaces/extension_wrapper.ts +++ b/packages/nodejs/src/interfaces/extension_wrapper.ts @@ -1,4 +1,5 @@ export interface ExtensionWrapper { + isLoaded: boolean extension: any span: any datamap: any