Make misconfigured telemetry for function parameters log the error, and not blow up fatally

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

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;

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;

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", () => {

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"

packages/core/src/main/logger/console-transport.injectable.ts

@@ -30,6 +30,7 @@ const consoleLoggerTransportInjectable = getInjectable({
     ),
   }),
   injectionToken: loggerTransportInjectionToken,
+  decorable: false,
 });
 
 export default consoleLoggerTransportInjectable;

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;

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;