From c6799e1478f4064a72e9a3aef21ab8b84857bd51 Mon Sep 17 00:00:00 2001 From: Iku-turso Date: Mon, 6 Mar 2023 13:14:27 +0200 Subject: [PATCH] White-listing of telemetry params (#7262) * Drop support for adding telemetry by tagging This was not used, and would make development of future feature more difficult. Co-authored-by: Janne Savolainen Signed-off-by: Iku-turso * Introduce white-listing for params of telemetry Co-authored-by: Janne Savolainen Signed-off-by: Iku-turso * Fix linting Co-authored-by: Janne Savolainen Signed-off-by: Iku-turso * Make misconfigured telemetry for function parameters log the error, and not blow up fatally Co-authored-by: Janne Savolainen Signed-off-by: Iku-turso --------- Signed-off-by: Iku-turso --- .../core/src/common/log-error.injectable.ts | 1 + packages/core/src/common/logger.injectable.ts | 2 + ...metry-from-specific-function-calls.test.ts | 167 +++++++++++++----- .../renderer/emit-telemetry.injectable.ts | 6 +- .../telemetry-decorator.injectable.ts | 64 +++++-- ...try-white-list-for-functions.injectable.ts | 19 +- .../logger/console-transport.injectable.ts | 1 + .../main/logger/file-transport.injectable.ts | 1 + .../logger/browser-transport.injectable.ts | 1 + 9 files changed, 189 insertions(+), 73 deletions(-) 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 961647fa29..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; @@ -22,72 +23,158 @@ describe("emit-telemetry-from-specific-function-calls", () => { beforeEach(() => { di.override(telemetryWhiteListForFunctionsInjectable, () => [ "some-white-listed-function", + + { + 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 and tagged functions", () => { + describe("given instances of white-listed and non-white-listed functions", () => { let whiteListedFunctionMock: jest.Mock; let nonWhiteListedFunctionMock: jest.Mock; - let taggedFunctionMock: jest.Mock; - let injectedWhiteListedFunction: jest.Mock; - let injectedNonWhiteListedFunction: jest.Mock; - let injectedTaggedFunction: 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(); - taggedFunctionMock = jest.fn(); + logErrorMock = jest.fn(); const whiteListedInjectable = getInjectable({ id: "some-white-listed-function", instantiate: () => whiteListedFunctionMock, }); + const whiteListedInjectableWithArgument = getInjectable({ + id: "some-white-listed-function-with-white-listed-argument", + 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, }); - const taggedInjectable = getInjectable({ - id: "some-tagged-function", - instantiate: () => taggedFunctionMock, - tags: ["emit-telemetry"], - }); - runInAction(() => { - di.register(whiteListedInjectable); - di.register(nonWhiteListedInjectable); - di.register(taggedInjectable); + di.register( + whiteListedInjectable, + whiteListedInjectableWithArgument, + whiteListedInjectableWithBadConfig, + nonWhiteListedInjectable, + ); }); - injectedWhiteListedFunction = di.inject(whiteListedInjectable); - injectedNonWhiteListedFunction = di.inject(nonWhiteListedInjectable); - injectedTaggedFunction = di.inject(taggedInjectable); + di.override(logErrorInjectable, () => logErrorMock); + + whiteListedFunction = di.inject(whiteListedInjectable); + + whiteListedFunctionWithArgument = di.inject( + whiteListedInjectableWithArgument, + ); + + whiteListedFunctionWithFaultyConfig = di.inject( + whiteListedInjectableWithBadConfig, + ); + + nonWhiteListedFunction = di.inject(nonWhiteListedInjectable); }); it("telemetry is not emitted yet", () => { expect(emitEventMock).not.toHaveBeenCalled(); }); - describe("when the white-listed function is called", () => { + 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", () => { + it("telemetry is emitted in event bus without the arguments", () => { expect(emitEventMock).toHaveBeenCalledWith({ destination: "auto-capture", action: "telemetry-from-business-action", name: "some-white-listed-function", - params: { args: ["some-arg", "some-other-arg"] }, }); }); }); - describe("when the white-listed function is called with MobX reactive content", () => { + describe("when a white-listed function with a white-listed argument is called with arguments", () => { + beforeEach(() => { + whiteListedFunctionWithArgument("some-arg", "some-other-arg"); + }); + + it("telemetry is emitted in event bus with the arguments as params", () => { + expect(emitEventMock).toHaveBeenCalledWith({ + action: "telemetry-from-business-action", + destination: "auto-capture", + name: "some-white-listed-function-with-white-listed-argument", + params: { someParam: "some-other-arg" }, + }); + }); + }); + + describe("when a white-listed function with a white-listed argument is called without arguments", () => { + beforeEach(() => { + whiteListedFunctionWithArgument(); + }); + + it("telemetry is emitted in event bus without params", () => { + expect(emitEventMock).toHaveBeenCalledWith({ + action: "telemetry-from-business-action", + destination: "auto-capture", + name: "some-white-listed-function-with-white-listed-argument", + params: { someParam: undefined }, + }); + }); + }); + + 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"); @@ -96,22 +183,23 @@ describe("emit-telemetry-from-specific-function-calls", () => { someComputedProperty, }; - injectedWhiteListedFunction(someObservable); + whiteListedFunctionWithArgument( + "irrelevant-argument", + someObservable, + ); }); it("telemetry is emitted in event bus without MobX internals or computeds", () => { expect(emitEventMock).toHaveBeenCalledWith({ destination: "auto-capture", action: "telemetry-from-business-action", - name: "some-white-listed-function", + name: "some-white-listed-function-with-white-listed-argument", params: { - args: [ - { - someStaticProperty: "some-static-value", - someComputedProperty: "some-computed-value", - }, - ], + someParam: { + someStaticProperty: "some-static-value", + someComputedProperty: "some-computed-value", + }, }, }); }); @@ -119,28 +207,13 @@ describe("emit-telemetry-from-specific-function-calls", () => { describe("when the non-white-listed function is called", () => { beforeEach(() => { - injectedNonWhiteListedFunction(); + nonWhiteListedFunction(); }); it("telemetry is not emitted", () => { expect(emitEventMock).not.toHaveBeenCalled(); }); }); - - describe("when the tagged, but not white-listed function is called", () => { - beforeEach(() => { - injectedTaggedFunction("some-arg", "some-other-arg"); - }); - - it("telemetry is emitted in event bus", () => { - expect(emitEventMock).toHaveBeenCalledWith({ - destination: "auto-capture", - action: "telemetry-from-business-action", - name: "some-tagged-function", - params: { args: ["some-arg", "some-other-arg"] }, - }); - }); - }); }); }); }); diff --git a/packages/core/src/features/telemetry/renderer/emit-telemetry.injectable.ts b/packages/core/src/features/telemetry/renderer/emit-telemetry.injectable.ts index f543daf843..5fbc2f4b38 100644 --- a/packages/core/src/features/telemetry/renderer/emit-telemetry.injectable.ts +++ b/packages/core/src/features/telemetry/renderer/emit-telemetry.injectable.ts @@ -4,7 +4,7 @@ */ import { getInjectable } from "@ogre-tools/injectable"; import emitEventInjectable from "../../../common/app-event-bus/emit-event.injectable"; -import { toJS, observable } from "mobx"; +import { observable, toJS } from "mobx"; const emitTelemetryInjectable = getInjectable({ id: "emit-telemetry", @@ -12,12 +12,12 @@ const emitTelemetryInjectable = getInjectable({ instantiate: (di) => { const emitEvent = di.inject(emitEventInjectable); - return ({ action, args }: { action: string; args: any[] }) => { + return ({ action, params }: { action: string; params?: object }) => { emitEvent({ destination: "auto-capture", action: "telemetry-from-business-action", name: action, - params: { args: toJS(observable(args)) }, + ...(params ? { params: toJS(observable(params)) } : {}), }); }; }, 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 e1a614660c..ad15cf84ef 100644 --- a/packages/core/src/features/telemetry/renderer/telemetry-decorator.injectable.ts +++ b/packages/core/src/features/telemetry/renderer/telemetry-decorator.injectable.ts @@ -2,33 +2,34 @@ * Copyright (c) OpenLens Authors. All rights reserved. * Licensed under MIT License. See LICENSE in root directory for more information. */ -import type { - DiContainerForInjection, - Injectable, -} from "@ogre-tools/injectable"; +import type { DiContainerForInjection } from "@ogre-tools/injectable"; import { - lifecycleEnum, getInjectable, instantiationDecoratorToken, + lifecycleEnum, } from "@ogre-tools/injectable"; -import assert from "assert"; +import assert from "assert"; import { isFunction } from "lodash/fp"; 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 shouldEmitTelemetry = shouldEmitTelemetryFor(whiteList); + const whiteListMap = getWhiteListMap(whiteList); return { decorate: @@ -42,8 +43,31 @@ const telemetryDecoratorInjectable = getInjectable({ assert(currentContext); - if (shouldEmitTelemetry(currentContext.injectable)) { - emitTelemetry({ action: currentContext.injectable.id, args }); + 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, + }); } return instance(...args); @@ -61,9 +85,23 @@ const telemetryDecoratorInjectable = getInjectable({ injectionToken: instantiationDecoratorToken, }); -const shouldEmitTelemetryFor = - (whiteList: string[]) => (injectable: Injectable) => - injectable.tags?.includes("emit-telemetry") || - whiteList.includes(injectable.id); +const getWhiteListMap = (whiteList: WhiteListItem[]) => + new Map( + whiteList.map((item) => + typeof item === "string" + ? [ + item, + { + getParams: () => undefined, + }, + ] + : [ + item.id, + { + getParams: item.getParams, + }, + ], + ), + ); export default telemetryDecoratorInjectable; diff --git a/packages/core/src/features/telemetry/renderer/telemetry-white-list-for-functions.injectable.ts b/packages/core/src/features/telemetry/renderer/telemetry-white-list-for-functions.injectable.ts index 38f0bb6ca6..0d2970f75e 100644 --- a/packages/core/src/features/telemetry/renderer/telemetry-white-list-for-functions.injectable.ts +++ b/packages/core/src/features/telemetry/renderer/telemetry-white-list-for-functions.injectable.ts @@ -3,6 +3,7 @@ * Licensed under MIT License. See LICENSE in root directory for more information. */ import { getInjectable } from "@ogre-tools/injectable"; +import type { AppEvent } from "../../../common/app-event-bus/event-bus"; const navigateTo = [ "navigate-to-preference-tab-id", @@ -88,21 +89,19 @@ const extensions = [ "uninstall-extension", ]; -const externalActions = [ - "open-link-in-browser", -]; +const externalActions = ["open-link-in-browser"]; -const uiInteraction = [ - "show-details", -]; +const uiInteraction = ["show-details"]; -const terminal = [ - "create-terminal-tab", -]; +const terminal = ["create-terminal-tab"]; + +export type WhiteListItem = + | string + | { id: string; getParams: (...args: unknown[]) => AppEvent["params"] }; const telemetryWhiteListForFunctionsInjectable = getInjectable({ id: "telemetry-white-list-for-functions", - instantiate: () => [ + instantiate: (): WhiteListItem[] => [ ...navigateTo, ...helmInjectableIds, ...kubeConfigActions, 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;