From 56fb794a72e99d77f787000bf2a5bf95efc44565 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Tue, 18 Jan 2022 14:38:08 +0100 Subject: [PATCH] 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 --- ...change-the-way-the-extension-is-started.md | 6 +++ .../.changesets/improve-extension-loading.md | 6 +++ packages/nodejs/src/__tests__/client.test.ts | 13 ++++-- .../src/__tests__/extension.failure.test.ts | 33 +++++++++++++++ .../nodejs/src/__tests__/extension.test.ts | 16 ++----- packages/nodejs/src/client.ts | 9 ++-- packages/nodejs/src/diagnose.ts | 4 +- packages/nodejs/src/extension.ts | 42 +++++++------------ packages/nodejs/src/extension_wrapper.ts | 2 + .../src/interfaces/extension_wrapper.ts | 1 + 10 files changed, 80 insertions(+), 52 deletions(-) create mode 100644 packages/nodejs/.changesets/change-the-way-the-extension-is-started.md create mode 100644 packages/nodejs/.changesets/improve-extension-loading.md create mode 100644 packages/nodejs/src/__tests__/extension.failure.test.ts 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 00000000..6776eab4 --- /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 00000000..3d5ecadf --- /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 1bcf7538..b2745cc8 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 00000000..b4738e63 --- /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 46c98f2a..f4dda180 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 342b436b..fd4dbea6 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 266c350b..a246a981 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 bb495451..bf12b261 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 622cd2c8..c2510d97 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 29601586..3daa899c 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