From 6bf596c7f07fec6652e8c9aa64cc1604b9e984b3 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Tue, 7 May 2024 18:46:51 +0200 Subject: [PATCH] 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. --- ...e-error-reporting-during-initialisation.md | 10 +++ src/__tests__/client.test.ts | 8 +- src/__tests__/config.test.ts | 25 +++--- src/__tests__/extension.failure.test.ts | 24 ++++-- src/client.ts | 85 +++++++++---------- src/config.ts | 15 +++- src/extension.ts | 4 + src/extension_wrapper.ts | 39 ++++----- 8 files changed, 123 insertions(+), 87 deletions(-) create mode 100644 .changesets/improve-error-reporting-during-initialisation.md diff --git a/.changesets/improve-error-reporting-during-initialisation.md b/.changesets/improve-error-reporting-during-initialisation.md new file mode 100644 index 00000000..db56d1b0 --- /dev/null +++ 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 index c5f18207..bdfab06c 100644 --- 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 index a10e6554..1af8d592 100644 --- 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 index c63e8775..e564a3aa 100644 --- 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 support@appsignal.com: 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 support@appsignal.com: 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 index cca2f939..97e55c0e 100644 --- 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 = {}) { 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 index d230792f..6439e878 100644 --- 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 index 85e0c145..43336db4 100644 --- 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 index 2eb631c8..1ed7ce6a 100644 --- 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 support@appsignal.com: 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 support@appsignal.com: https://docs.appsignal.com/nodejs/3.x/command-line/diagnose.html\n", + error + ) + } } } } as ExtensionWrapper