diff --git a/packages/core/src/common/log-error.injectable.ts b/packages/core/src/common/log-error.injectable.ts index 4fab2cd546..84a77a679a 100644 --- a/packages/core/src/common/log-error.injectable.ts +++ b/packages/core/src/common/log-error.injectable.ts @@ -8,6 +8,7 @@ import loggerInjectable from "./logger.injectable"; const logErrorInjectable = getInjectable({ id: "log-error", instantiate: (di) => di.inject(loggerInjectable).error, + decorable: false, }); export default logErrorInjectable; diff --git a/packages/core/src/common/logger.injectable.ts b/packages/core/src/common/logger.injectable.ts index 8e9dd2a6a7..bc1c5de71b 100644 --- a/packages/core/src/common/logger.injectable.ts +++ b/packages/core/src/common/logger.injectable.ts @@ -26,6 +26,8 @@ const loggerInjectable = getInjectable({ silly: (message, ...data) => baseLogger.silly(message, ...data), }; }, + + decorable: false, }); export default loggerInjectable; diff --git a/packages/core/src/features/telemetry/emit-telemetry-from-specific-function-calls.test.ts b/packages/core/src/features/telemetry/emit-telemetry-from-specific-function-calls.test.ts index 0463e5aefc..c6ea8b25e2 100644 --- a/packages/core/src/features/telemetry/emit-telemetry-from-specific-function-calls.test.ts +++ b/packages/core/src/features/telemetry/emit-telemetry-from-specific-function-calls.test.ts @@ -8,6 +8,7 @@ import { getInjectable } from "@ogre-tools/injectable"; import { getDiForUnitTesting } from "../../renderer/getDiForUnitTesting"; import telemetryWhiteListForFunctionsInjectable from "./renderer/telemetry-white-list-for-functions.injectable"; import emitEventInjectable from "../../common/app-event-bus/emit-event.injectable"; +import logErrorInjectable from "../../common/log-error.injectable"; describe("emit-telemetry-from-specific-function-calls", () => { let di: DiContainer; @@ -27,22 +28,33 @@ describe("emit-telemetry-from-specific-function-calls", () => { id: "some-white-listed-function-with-white-listed-argument", getParams: (irrelevantArg, arg) => ({ someParam: arg }), }, + + { + id: "some-white-listed-function-with-bad-config", + + getParams: () => { + throw new Error("some-error-from-bad-configuration"); + }, + }, ]); emitEventMock = jest.fn(); di.override(emitEventInjectable, () => emitEventMock); }); - describe("given instances of white-listed, non-white-listed", () => { + describe("given instances of white-listed and non-white-listed functions", () => { let whiteListedFunctionMock: jest.Mock; let nonWhiteListedFunctionMock: jest.Mock; - let injectedWhiteListedFunction: jest.Mock; - let injectedWhiteListedFunctionWithArgument: jest.Mock; - let injectedNonWhiteListedFunction: jest.Mock; + let whiteListedFunction: jest.Mock; + let whiteListedFunctionWithArgument: jest.Mock; + let whiteListedFunctionWithFaultyConfig: jest.Mock; + let nonWhiteListedFunction: jest.Mock; + let logErrorMock: jest.Mock; beforeEach(() => { whiteListedFunctionMock = jest.fn(); nonWhiteListedFunctionMock = jest.fn(); + logErrorMock = jest.fn(); const whiteListedInjectable = getInjectable({ id: "some-white-listed-function", @@ -54,6 +66,11 @@ describe("emit-telemetry-from-specific-function-calls", () => { instantiate: () => whiteListedFunctionMock, }); + const whiteListedInjectableWithBadConfig = getInjectable({ + id: "some-white-listed-function-with-bad-config", + instantiate: () => whiteListedFunctionMock, + }); + const nonWhiteListedInjectable = getInjectable({ id: "some-non-white-listed-function", instantiate: () => nonWhiteListedFunctionMock, @@ -63,26 +80,37 @@ describe("emit-telemetry-from-specific-function-calls", () => { di.register( whiteListedInjectable, whiteListedInjectableWithArgument, + whiteListedInjectableWithBadConfig, nonWhiteListedInjectable, ); }); - injectedWhiteListedFunction = di.inject(whiteListedInjectable); + di.override(logErrorInjectable, () => logErrorMock); - injectedWhiteListedFunctionWithArgument = di.inject( + whiteListedFunction = di.inject(whiteListedInjectable); + + whiteListedFunctionWithArgument = di.inject( whiteListedInjectableWithArgument, ); - injectedNonWhiteListedFunction = di.inject(nonWhiteListedInjectable); + whiteListedFunctionWithFaultyConfig = di.inject( + whiteListedInjectableWithBadConfig, + ); + + nonWhiteListedFunction = di.inject(nonWhiteListedInjectable); }); it("telemetry is not emitted yet", () => { expect(emitEventMock).not.toHaveBeenCalled(); }); + it("doesn't log errors, at least yet", () => { + expect(logErrorMock).not.toHaveBeenCalled(); + }); + describe("when a normal white-listed function is called with arguments", () => { beforeEach(() => { - injectedWhiteListedFunction("some-arg", "some-other-arg"); + whiteListedFunction("some-arg", "some-other-arg"); }); it("telemetry is emitted in event bus without the arguments", () => { @@ -96,7 +124,7 @@ describe("emit-telemetry-from-specific-function-calls", () => { describe("when a white-listed function with a white-listed argument is called with arguments", () => { beforeEach(() => { - injectedWhiteListedFunctionWithArgument("some-arg", "some-other-arg"); + whiteListedFunctionWithArgument("some-arg", "some-other-arg"); }); it("telemetry is emitted in event bus with the arguments as params", () => { @@ -111,7 +139,7 @@ describe("emit-telemetry-from-specific-function-calls", () => { describe("when a white-listed function with a white-listed argument is called without arguments", () => { beforeEach(() => { - injectedWhiteListedFunctionWithArgument(); + whiteListedFunctionWithArgument(); }); it("telemetry is emitted in event bus without params", () => { @@ -124,7 +152,29 @@ describe("emit-telemetry-from-specific-function-calls", () => { }); }); - describe("when the white-listed function with a white-listed argument is called with MobX reactive content", () => { + describe("given a faulty configuration, when a white-listed function is called", () => { + beforeEach(() => { + whiteListedFunctionWithFaultyConfig(); + }); + + it("telemetry is still emitted in event bus, but with params indicating bad configuration, ", () => { + expect(emitEventMock).toHaveBeenCalledWith({ + action: "telemetry-from-business-action", + destination: "auto-capture", + name: "some-white-listed-function-with-bad-config", + params: { error: "Tried to produce params for telemetry, but getParams() threw an error" }, + }); + }); + + it("logs error", () => { + expect(logErrorMock).toHaveBeenCalledWith( + 'Tried to produce params for telemetry of "some-white-listed-function-with-bad-config", but getParams() threw an error', + expect.objectContaining({ message: "some-error-from-bad-configuration" }), + ); + }); + }); + + describe("when a white-listed function with a white-listed argument is called with MobX reactive content", () => { beforeEach(() => { const someComputedProperty = computed(() => "some-computed-value"); @@ -133,7 +183,7 @@ describe("emit-telemetry-from-specific-function-calls", () => { someComputedProperty, }; - injectedWhiteListedFunctionWithArgument( + whiteListedFunctionWithArgument( "irrelevant-argument", someObservable, ); @@ -157,7 +207,7 @@ describe("emit-telemetry-from-specific-function-calls", () => { describe("when the non-white-listed function is called", () => { beforeEach(() => { - injectedNonWhiteListedFunction(); + nonWhiteListedFunction(); }); it("telemetry is not emitted", () => { diff --git a/packages/core/src/features/telemetry/renderer/telemetry-decorator.injectable.ts b/packages/core/src/features/telemetry/renderer/telemetry-decorator.injectable.ts index 7e2520bee5..ad15cf84ef 100644 --- a/packages/core/src/features/telemetry/renderer/telemetry-decorator.injectable.ts +++ b/packages/core/src/features/telemetry/renderer/telemetry-decorator.injectable.ts @@ -16,18 +16,20 @@ import emitTelemetryInjectable from "./emit-telemetry.injectable"; import type { WhiteListItem } from "./telemetry-white-list-for-functions.injectable"; import telemetryWhiteListForFunctionsInjectable from "./telemetry-white-list-for-functions.injectable"; +import logErrorInjectable from "../../../common/log-error.injectable"; const telemetryDecoratorInjectable = getInjectable({ id: "telemetry-decorator", instantiate: (diForDecorator) => { const emitTelemetry = diForDecorator.inject(emitTelemetryInjectable); + const logError = diForDecorator.inject(logErrorInjectable); const whiteList = diForDecorator.inject( telemetryWhiteListForFunctionsInjectable, ); - const whitleListMap = getWhitleListMap(whiteList); + const whiteListMap = getWhiteListMap(whiteList); return { decorate: @@ -41,14 +43,30 @@ const telemetryDecoratorInjectable = getInjectable({ assert(currentContext); - const whiteListed = whitleListMap.get( + const whiteListed = whiteListMap.get( currentContext.injectable.id, ); if (whiteListed) { + let params; + + try { + params = whiteListed.getParams(...args); + } catch (e) { + params = { + error: + "Tried to produce params for telemetry, but getParams() threw an error", + }; + + logError( + `Tried to produce params for telemetry of "${currentContext.injectable.id}", but getParams() threw an error`, + e, + ); + } + emitTelemetry({ action: currentContext.injectable.id, - params: whiteListed.getParams(...args), + params, }); } @@ -67,7 +85,7 @@ const telemetryDecoratorInjectable = getInjectable({ injectionToken: instantiationDecoratorToken, }); -const getWhitleListMap = (whiteList: WhiteListItem[]) => +const getWhiteListMap = (whiteList: WhiteListItem[]) => new Map( whiteList.map((item) => typeof item === "string" diff --git a/packages/core/src/main/logger/console-transport.injectable.ts b/packages/core/src/main/logger/console-transport.injectable.ts index 8200eafe2a..1e70b92be9 100644 --- a/packages/core/src/main/logger/console-transport.injectable.ts +++ b/packages/core/src/main/logger/console-transport.injectable.ts @@ -30,6 +30,7 @@ const consoleLoggerTransportInjectable = getInjectable({ ), }), injectionToken: loggerTransportInjectionToken, + decorable: false, }); export default consoleLoggerTransportInjectable; diff --git a/packages/core/src/main/logger/file-transport.injectable.ts b/packages/core/src/main/logger/file-transport.injectable.ts index 2f1eab5c0c..fcf855eec4 100644 --- a/packages/core/src/main/logger/file-transport.injectable.ts +++ b/packages/core/src/main/logger/file-transport.injectable.ts @@ -23,6 +23,7 @@ const fileLoggerTranportInjectable = getInjectable({ tailable: true, }), injectionToken: loggerTransportInjectionToken, + decorable: false, }); export default fileLoggerTranportInjectable; diff --git a/packages/core/src/renderer/logger/browser-transport.injectable.ts b/packages/core/src/renderer/logger/browser-transport.injectable.ts index 5a38c5a396..80aafc99c5 100644 --- a/packages/core/src/renderer/logger/browser-transport.injectable.ts +++ b/packages/core/src/renderer/logger/browser-transport.injectable.ts @@ -10,6 +10,7 @@ const browserLoggerTransportInjectable = getInjectable({ id: "browser-logger-transport", instantiate: () => new BrowserConsole(), injectionToken: loggerTransportInjectionToken, + decorable: false, }); export default browserLoggerTransportInjectable;