From 91a40c6b62983a1680d148cd83984442fed8cd1c Mon Sep 17 00:00:00 2001 From: Sami Tiilikainen <97873007+samitiilikainen@users.noreply.github.com> Date: Fri, 17 Mar 2023 13:18:56 +0200 Subject: [PATCH] Review improvements Signed-off-by: Sami Tiilikainen <97873007+samitiilikainen@users.noreply.github.com> --- .../close-ipc-logging-listener.injectable.ts | 9 +-- ...ransport.global-override-for-injectable.ts | 18 ++++++ .../create-ipc-file-transport.injectable.ts | 28 +++++++++ .../main/logger/file-transport.injectable.ts | 2 +- .../main/logger/ipc-file-logger.injectable.ts | 58 +++++++++++++----- .../src/main/logger/ipc-file-logger.test.ts | 59 ++++++------------- .../core/src/main/logger/ipc-file-logger.ts | 56 ------------------ .../logger/ipc-logging-listener.injectable.ts | 17 +++--- .../logger/renderer-log-file-id.injectable.ts | 2 +- 9 files changed, 122 insertions(+), 127 deletions(-) create mode 100644 packages/core/src/main/logger/create-ipc-file-transport.global-override-for-injectable.ts create mode 100644 packages/core/src/main/logger/create-ipc-file-transport.injectable.ts delete mode 100644 packages/core/src/main/logger/ipc-file-logger.ts diff --git a/packages/core/src/main/logger/close-ipc-logging-listener.injectable.ts b/packages/core/src/main/logger/close-ipc-logging-listener.injectable.ts index 606b81a181..6870a29c61 100644 --- a/packages/core/src/main/logger/close-ipc-logging-listener.injectable.ts +++ b/packages/core/src/main/logger/close-ipc-logging-listener.injectable.ts @@ -11,10 +11,11 @@ import { const closeIpcFileLoggingListenerInjectable = getMessageChannelListenerInjectable({ id: "close-ipc-file-logging", channel: closeIpcFileLoggerChannel, - handler: (di) => (fileId) => - di - .inject(ipcFileLoggerInjectable) - .close(fileId), + handler: (di) => { + const ipcFileLogger = di.inject(ipcFileLoggerInjectable); + + return (fileId) => ipcFileLogger.close(fileId); + }, }); export default closeIpcFileLoggingListenerInjectable; diff --git a/packages/core/src/main/logger/create-ipc-file-transport.global-override-for-injectable.ts b/packages/core/src/main/logger/create-ipc-file-transport.global-override-for-injectable.ts new file mode 100644 index 0000000000..ba6f57283f --- /dev/null +++ b/packages/core/src/main/logger/create-ipc-file-transport.global-override-for-injectable.ts @@ -0,0 +1,18 @@ +/** + * Copyright (c) OpenLens Authors. All rights reserved. + * Licensed under MIT License. See LICENSE in root directory for more information. + */ + +import type { transports } from "winston"; +import { getGlobalOverride } from "@k8slens/test-utils"; +import { noop } from "@k8slens/utilities"; +import createIpcFileLoggerTranportInjectable from "./create-ipc-file-transport.injectable"; + +export default getGlobalOverride( + createIpcFileLoggerTranportInjectable, + () => () => + ({ + log: noop, + close: noop, + } as typeof transports.File), +); diff --git a/packages/core/src/main/logger/create-ipc-file-transport.injectable.ts b/packages/core/src/main/logger/create-ipc-file-transport.injectable.ts new file mode 100644 index 0000000000..da05476010 --- /dev/null +++ b/packages/core/src/main/logger/create-ipc-file-transport.injectable.ts @@ -0,0 +1,28 @@ +/** + * Copyright (c) OpenLens Authors. All rights reserved. + * Licensed under MIT License. See LICENSE in root directory for more information. + */ +import { getInjectable } from "@ogre-tools/injectable"; +import { transports } from "winston"; +import directoryForLogsInjectable from "../../common/app-paths/directory-for-logs.injectable"; + +const createIpcFileLoggerTranportInjectable = getInjectable({ + id: "create-ipc-file-logger-transport", + instantiate: (di) => { + const options = { + dirname: di.inject(directoryForLogsInjectable), + maxsize: 1024 * 1024, + maxFiles: 2, + tailable: true, + }; + + return (fileId: string) => + new transports.File({ + ...options, + filename: `lens-${fileId}.log`, + }); + }, + causesSideEffects: true, +}); + +export default createIpcFileLoggerTranportInjectable; diff --git a/packages/core/src/main/logger/file-transport.injectable.ts b/packages/core/src/main/logger/file-transport.injectable.ts index fcf855eec4..09d4a4631c 100644 --- a/packages/core/src/main/logger/file-transport.injectable.ts +++ b/packages/core/src/main/logger/file-transport.injectable.ts @@ -8,7 +8,7 @@ import directoryForLogsInjectable from "../../common/app-paths/directory-for-log import { loggerTransportInjectionToken } from "../../common/logger/transports"; const fileLoggerTranportInjectable = getInjectable({ - id: "file-logger-tranport", + id: "file-logger-transport", instantiate: (di) => new transports.File({ handleExceptions: false, level: "debug", diff --git a/packages/core/src/main/logger/ipc-file-logger.injectable.ts b/packages/core/src/main/logger/ipc-file-logger.injectable.ts index dad6723f99..49c9041b61 100644 --- a/packages/core/src/main/logger/ipc-file-logger.injectable.ts +++ b/packages/core/src/main/logger/ipc-file-logger.injectable.ts @@ -3,23 +3,53 @@ * Licensed under MIT License. See LICENSE in root directory for more information. */ import { getInjectable } from "@ogre-tools/injectable"; -import { transports } from "winston"; -import directoryForLogsInjectable from "../../common/app-paths/directory-for-logs.injectable"; -import IpcFileLogger from "./ipc-file-logger"; +import { getOrInsertWith } from "@k8slens/utilities"; +import type { LogEntry, transports } from "winston"; +import createIpcFileLoggerTranportInjectable from "./create-ipc-file-transport.injectable"; + +export interface IpcFileLogger { + log: (fileLog: { fileId: string; entry: LogEntry }) => void; + close: (fileId: string) => void; + closeAll: () => void; +} const ipcFileLoggerInjectable = getInjectable({ id: "ipc-file-logger", - instantiate: (di) => - new IpcFileLogger( - { - dirname: di.inject(directoryForLogsInjectable), - maxsize: 1024 * 1024, - maxFiles: 2, - tailable: true, - }, - (options: transports.FileTransportOptions) => new transports.File(options), - ), - causesSideEffects: true, + instantiate: (di): IpcFileLogger => { + const createIpcFileTransport = di.inject(createIpcFileLoggerTranportInjectable); + const fileTransports = new Map(); + + function log({ fileId, entry }: { fileId: string; entry: LogEntry }) { + const transport = getOrInsertWith( + fileTransports, + fileId, + () => createIpcFileTransport(fileId), + ); + + transport?.log?.(entry, () => {}); + } + + function close(fileId: string) { + const transport = fileTransports.get(fileId); + + if (transport) { + transport.close?.(); + fileTransports.delete(fileId); + } + } + + function closeAll() { + for (const fileId of fileTransports.keys()) { + close(fileId); + } + } + + return { + log, + close, + closeAll, + }; + }, }); export default ipcFileLoggerInjectable; diff --git a/packages/core/src/main/logger/ipc-file-logger.test.ts b/packages/core/src/main/logger/ipc-file-logger.test.ts index 0c292fcb16..35ad57c04a 100644 --- a/packages/core/src/main/logger/ipc-file-logger.test.ts +++ b/packages/core/src/main/logger/ipc-file-logger.test.ts @@ -2,7 +2,10 @@ * Copyright (c) OpenLens Authors. All rights reserved. * Licensed under MIT License. See LICENSE in root directory for more information. */ -import IpcFileLogger from "./ipc-file-logger"; +import { getDiForUnitTesting } from "../getDiForUnitTesting"; +import createIpcFileLoggerTranportInjectable from "./create-ipc-file-transport.injectable"; +import type { IpcFileLogger } from "./ipc-file-logger.injectable"; +import ipcFileLoggerInjectable from "./ipc-file-logger.injectable"; describe("ipc file logger in main", () => { let logMock: jest.Mock; @@ -17,14 +20,11 @@ describe("ipc file logger in main", () => { log: logMock, close: closeMock, })); - logger = new IpcFileLogger( - { - dirname: "some-logs-dir", - maxFiles: 1, - tailable: true, - }, - createFileTransportMock, - ); + + const di = getDiForUnitTesting(); + + di.override(createIpcFileLoggerTranportInjectable, () => createFileTransportMock); + logger = di.inject(ipcFileLoggerInjectable); }); it("creates a transport for new log file", () => { @@ -33,12 +33,7 @@ describe("ipc file logger in main", () => { entry: { level: "irrelevant", message: "irrelevant" }, }); - expect(createFileTransportMock).toHaveBeenCalledWith({ - dirname: "some-logs-dir", - filename: "lens-some-log-file.log", - maxFiles: 1, - tailable: true, - }); + expect(createFileTransportMock).toHaveBeenCalledWith("some-log-file"); }); it("uses existing transport for log file", () => { @@ -59,12 +54,7 @@ describe("ipc file logger in main", () => { expect(createFileTransportMock).toHaveBeenCalledTimes(1); - expect(createFileTransportMock).toHaveBeenCalledWith({ - dirname: "some-logs-dir", - filename: "lens-some-log-file.log", - maxFiles: 1, - tailable: true, - }); + expect(createFileTransportMock).toHaveBeenCalledWith("some-log-file"); }); it("creates separate transport for each log file", () => { @@ -85,26 +75,11 @@ describe("ipc file logger in main", () => { expect(createFileTransportMock).toHaveBeenCalledTimes(3); - expect(createFileTransportMock).toHaveBeenCalledWith({ - dirname: "some-logs-dir", - filename: "lens-some-log-file.log", - maxFiles: 1, - tailable: true, - }); + expect(createFileTransportMock).toHaveBeenCalledWith("some-log-file"); - expect(createFileTransportMock).toHaveBeenCalledWith({ - dirname: "some-logs-dir", - filename: "lens-some-other-log-file.log", - maxFiles: 1, - tailable: true, - }); + expect(createFileTransportMock).toHaveBeenCalledWith("some-other-log-file"); - expect(createFileTransportMock).toHaveBeenCalledWith({ - dirname: "some-logs-dir", - filename: "lens-some-yet-another-log-file.log", - maxFiles: 1, - tailable: true, - }); + expect(createFileTransportMock).toHaveBeenCalledWith("some-yet-another-log-file"); }); it("logs using file transport", () => { @@ -122,12 +97,12 @@ describe("ipc file logger in main", () => { const someLogMock = jest.fn(); const someOthertLogMock = jest.fn(); - createFileTransportMock.mockImplementation((options) => { - if (options.filename === "lens-some-log-file.log") { + createFileTransportMock.mockImplementation((fileId: string) => { + if (fileId === "some-log-file") { return { log: someLogMock }; } - if (options.filename === "lens-some-other-log-file.log") { + if (fileId === "some-other-log-file") { return { log: someOthertLogMock }; } diff --git a/packages/core/src/main/logger/ipc-file-logger.ts b/packages/core/src/main/logger/ipc-file-logger.ts deleted file mode 100644 index 9babbc09db..0000000000 --- a/packages/core/src/main/logger/ipc-file-logger.ts +++ /dev/null @@ -1,56 +0,0 @@ -/** - * Copyright (c) OpenLens Authors. All rights reserved. - * Licensed under MIT License. See LICENSE in root directory for more information. - */ -import type { LogEntry, transports } from "winston"; - -type IpcFileLoggerOptions = Omit; - -class IpcFileLogger { - private fileTransports = new Map(); - - constructor( - private options: IpcFileLoggerOptions, - private createNewFileTransport: ( - options: transports.FileTransportOptions - ) => transports.FileTransportInstance, - ) {} - - log({ fileId, entry }: { fileId: string; entry: LogEntry }) { - const transport = this.ensureTransportForFile(fileId); - - transport?.log?.(entry, () => {}); - } - - close(fileId: string) { - const transport = this.fileTransports.get(fileId); - - if (transport) { - transport.close?.(); - this.fileTransports.delete(fileId); - } - } - - closeAll() { - [...this.fileTransports.keys()].forEach((fileId) => { - this.close(fileId); - }); - } - - private ensureTransportForFile(fileId: string) { - if (this.fileTransports.has(fileId)) { - return this.fileTransports.get(fileId); - } - - const fileTransport = this.createNewFileTransport({ - ...this.options, - filename: `lens-${fileId}.log`, - }); - - this.fileTransports.set(fileId, fileTransport); - - return fileTransport; - } -} - -export default IpcFileLogger; diff --git a/packages/core/src/main/logger/ipc-logging-listener.injectable.ts b/packages/core/src/main/logger/ipc-logging-listener.injectable.ts index 857e2ff6a1..3a3748846b 100644 --- a/packages/core/src/main/logger/ipc-logging-listener.injectable.ts +++ b/packages/core/src/main/logger/ipc-logging-listener.injectable.ts @@ -4,11 +4,8 @@ */ import ipcFileLoggerInjectable from "./ipc-file-logger.injectable"; import { getMessageChannelListenerInjectable } from "../../common/utils/channel/message-channel-listener-injection-token"; -import type { - IpcFileLogObject } from "../../common/logger/ipc-file-logger-channel"; -import { - ipcFileLoggerChannel, -} from "../../common/logger/ipc-file-logger-channel"; +import type { IpcFileLogObject } from "../../common/logger/ipc-file-logger-channel"; +import { ipcFileLoggerChannel } from "../../common/logger/ipc-file-logger-channel"; import { MESSAGE } from "triple-beam"; /** @@ -31,10 +28,12 @@ export function deserializeLogFromIpc(ipcFileLogObject: IpcFileLogObject) { const ipcFileLoggingListenerInjectable = getMessageChannelListenerInjectable({ id: "ipc-file-logging", channel: ipcFileLoggerChannel, - handler: (di) => (ipcFileLogObject) => - di - .inject(ipcFileLoggerInjectable) - .log(deserializeLogFromIpc(ipcFileLogObject)), + handler: (di) => { + const logger = di.inject(ipcFileLoggerInjectable); + + return (ipcFileLogObject) => + logger.log(deserializeLogFromIpc(ipcFileLogObject)); + }, }); export default ipcFileLoggingListenerInjectable; diff --git a/packages/core/src/renderer/logger/renderer-log-file-id.injectable.ts b/packages/core/src/renderer/logger/renderer-log-file-id.injectable.ts index 4a234afeea..81f4196b08 100644 --- a/packages/core/src/renderer/logger/renderer-log-file-id.injectable.ts +++ b/packages/core/src/renderer/logger/renderer-log-file-id.injectable.ts @@ -17,7 +17,7 @@ const rendererLogFileIdInjectable = getInjectable({ const { host } = di.inject(windowLocationInjectable); const clusterId = getClusterIdFromHost(host); - frameId = clusterId ? `cluster-${clusterId}` : "cluster"; + frameId = `cluster-${clusterId}`; } else { frameId = "main"; }