Ps/improve-log-service (#8989)

* Match console method signatures in logService abstraction

* Add a few usages of improved signature

* Remove reality check test

* Improve electron logging

apps/browser/src/background/runtime.background.ts

@@ -76,7 +76,8 @@ export default class RuntimeBackground {
 
       void this.processMessageWithSender(msg, sender).catch((err) =>
         this.logService.error(
-          `Error while processing message in RuntimeBackground '${msg?.command}'. Error: ${err?.message ?? "Unknown Error"}`,
+          `Error while processing message in RuntimeBackground '${msg?.command}'.`,
+          err,
         ),
       );
       return false;

apps/browser/src/platform/offscreen-document/offscreen-document.spec.ts

@@ -28,6 +28,7 @@ describe("OffscreenDocument", () => {
     });
 
     it("shows a console message if the handler throws an error", async () => {
+      const error = new Error("test error");
       browserClipboardServiceCopySpy.mockRejectedValueOnce(new Error("test error"));
 
       sendExtensionRuntimeMessage({ command: "offscreenCopyToClipboard", text: "test" });
@@ -35,7 +36,8 @@ describe("OffscreenDocument", () => {
 
       expect(browserClipboardServiceCopySpy).toHaveBeenCalled();
       expect(consoleErrorSpy).toHaveBeenCalledWith(
-        "Error resolving extension message response: Error: test error",
+        "Error resolving extension message response",
+        error,
       );
     });
 

apps/browser/src/platform/offscreen-document/offscreen-document.ts

@@ -71,7 +71,7 @@ class OffscreenDocument implements OffscreenDocumentInterface {
     Promise.resolve(messageResponse)
       .then((response) => sendResponse(response))
       .catch((error) =>
-        this.consoleLogService.error(`Error resolving extension message response: ${error}`),
+        this.consoleLogService.error("Error resolving extension message response", error),
       );
     return true;
   };

apps/cli/src/platform/services/console-log.service.spec.ts

@@ -2,13 +2,18 @@ import { interceptConsole, restoreConsole } from "@bitwarden/common/spec";
 
 import { ConsoleLogService } from "./console-log.service";
 
-let caughtMessage: any = {};
-
 describe("CLI Console log service", () => {
+  const error = new Error("this is an error");
+  const obj = { a: 1, b: 2 };
   let logService: ConsoleLogService;
+  let consoleSpy: {
+    log: jest.Mock<any, any>;
+    warn: jest.Mock<any, any>;
+    error: jest.Mock<any, any>;
+  };
+
   beforeEach(() => {
-    caughtMessage = {};
-    interceptConsole(caughtMessage);
+    consoleSpy = interceptConsole();
     logService = new ConsoleLogService(true);
   });
 
@@ -19,24 +24,21 @@ describe("CLI Console log service", () => {
   it("should redirect all console to error if BW_RESPONSE env is true", () => {
     process.env.BW_RESPONSE = "true";
 
-    logService.debug("this is a debug message");
-    expect(caughtMessage).toMatchObject({
-      error: { 0: "this is a debug message" },
-    });
+    logService.debug("this is a debug message", error, obj);
+    expect(consoleSpy.error).toHaveBeenCalledWith("this is a debug message", error, obj);
   });
 
   it("should not redirect console to error if BW_RESPONSE != true", () => {
     process.env.BW_RESPONSE = "false";
 
-    logService.debug("debug");
-    logService.info("info");
-    logService.warning("warning");
-    logService.error("error");
+    logService.debug("debug", error, obj);
+    logService.info("info", error, obj);
+    logService.warning("warning", error, obj);
+    logService.error("error", error, obj);
 
-    expect(caughtMessage).toMatchObject({
-      log: { 0: "info" },
-      warn: { 0: "warning" },
-      error: { 0: "error" },
-    });
+    expect(consoleSpy.log).toHaveBeenCalledWith("debug", error, obj);
+    expect(consoleSpy.log).toHaveBeenCalledWith("info", error, obj);
+    expect(consoleSpy.warn).toHaveBeenCalledWith("warning", error, obj);
+    expect(consoleSpy.error).toHaveBeenCalledWith("error", error, obj);
   });
 });

apps/cli/src/platform/services/console-log.service.ts

@@ -6,17 +6,17 @@ export class ConsoleLogService extends BaseConsoleLogService {
     super(isDev, filter);
   }
 
-  write(level: LogLevelType, message: string) {
+  write(level: LogLevelType, message?: any, ...optionalParams: any[]) {
     if (this.filter != null && this.filter(level)) {
       return;
     }
 
     if (process.env.BW_RESPONSE === "true") {
       // eslint-disable-next-line
-      console.error(message);
+      console.error(message, ...optionalParams);
       return;
     }
 
-    super.write(level, message);
+    super.write(level, message, ...optionalParams);
   }
 }

apps/desktop/src/platform/preload.ts

@@ -103,7 +103,8 @@ export default {
   isMacAppStore: isMacAppStore(),
   isWindowsStore: isWindowsStore(),
   reloadProcess: () => ipcRenderer.send("reload-process"),
-  log: (level: LogLevelType, message: string) => ipcRenderer.invoke("ipc.log", { level, message }),
+  log: (level: LogLevelType, message?: any, ...optionalParams: any[]) =>
+    ipcRenderer.invoke("ipc.log", { level, message, optionalParams }),
 
   openContextMenu: (
     menu: {

apps/desktop/src/platform/services/electron-log.main.service.ts

@@ -25,28 +25,28 @@ export class ElectronLogMainService extends BaseLogService {
     }
     log.initialize();
 
-    ipcMain.handle("ipc.log", (_event, { level, message }) => {
-      this.write(level, message);
+    ipcMain.handle("ipc.log", (_event, { level, message, optionalParams }) => {
+      this.write(level, message, ...optionalParams);
     });
   }
 
-  write(level: LogLevelType, message: string) {
+  write(level: LogLevelType, message?: any, ...optionalParams: any[]) {
     if (this.filter != null && this.filter(level)) {
       return;
     }
 
     switch (level) {
       case LogLevelType.Debug:
-        log.debug(message);
+        log.debug(message, ...optionalParams);
         break;
       case LogLevelType.Info:
-        log.info(message);
+        log.info(message, ...optionalParams);
         break;
       case LogLevelType.Warning:
-        log.warn(message);
+        log.warn(message, ...optionalParams);
         break;
       case LogLevelType.Error:
-        log.error(message);
+        log.error(message, ...optionalParams);
         break;
       default:
         break;

apps/desktop/src/platform/services/electron-log.renderer.service.ts

@@ -6,27 +6,29 @@ export class ElectronLogRendererService extends BaseLogService {
     super(ipc.platform.isDev, filter);
   }
 
-  write(level: LogLevelType, message: string) {
+  write(level: LogLevelType, message?: any, ...optionalParams: any[]) {
     if (this.filter != null && this.filter(level)) {
       return;
     }
 
     /* eslint-disable no-console */
-    ipc.platform.log(level, message).catch((e) => console.log("Error logging", e));
+    ipc.platform
+      .log(level, message, ...optionalParams)
+      .catch((e) => console.log("Error logging", e));
 
     /* eslint-disable no-console */
     switch (level) {
       case LogLevelType.Debug:
-        console.debug(message);
+        console.debug(message, ...optionalParams);
         break;
       case LogLevelType.Info:
-        console.info(message);
+        console.info(message, ...optionalParams);
         break;
       case LogLevelType.Warning:
-        console.warn(message);
+        console.warn(message, ...optionalParams);
         break;
       case LogLevelType.Error:
-        console.error(message);
+        console.error(message, ...optionalParams);
         break;
       default:
         break;

libs/angular/src/platform/services/logging-error-handler.ts

@@ -14,7 +14,7 @@ export class LoggingErrorHandler extends ErrorHandler {
   override handleError(error: any): void {
     try {
       const logService = this.injector.get(LogService, null);
-      logService.error(error);
+      logService.error("Unhandled error in angular", error);
     } catch {
       super.handleError(error);
     }

libs/common/spec/intercept-console.ts

@@ -2,22 +2,17 @@ const originalConsole = console;
 
 declare let console: any;
 
-export function interceptConsole(interceptions: any): object {
+export function interceptConsole(): {
+  log: jest.Mock<any, any>;
+  warn: jest.Mock<any, any>;
+  error: jest.Mock<any, any>;
+} {
   console = {
-    log: function () {
-      // eslint-disable-next-line
-      interceptions.log = arguments;
-    },
-    warn: function () {
-      // eslint-disable-next-line
-      interceptions.warn = arguments;
-    },
-    error: function () {
-      // eslint-disable-next-line
-      interceptions.error = arguments;
-    },
+    log: jest.fn(),
+    warn: jest.fn(),
+    error: jest.fn(),
   };
-  return interceptions;
+  return console;
 }
 
 export function restoreConsole() {

libs/common/src/platform/abstractions/log.service.ts

@@ -1,9 +1,9 @@
 import { LogLevelType } from "../enums/log-level-type.enum";
 
 export abstract class LogService {
-  abstract debug(message: string): void;
-  abstract info(message: string): void;
-  abstract warning(message: string): void;
-  abstract error(message: string): void;
-  abstract write(level: LogLevelType, message: string): void;
+  abstract debug(message?: any, ...optionalParams: any[]): void;
+  abstract info(message?: any, ...optionalParams: any[]): void;
+  abstract warning(message?: any, ...optionalParams: any[]): void;
+  abstract error(message?: any, ...optionalParams: any[]): void;
+  abstract write(level: LogLevelType, message?: any, ...optionalParams: any[]): void;
 }

libs/common/src/platform/services/console-log.service.spec.ts

@@ -2,13 +2,18 @@ import { interceptConsole, restoreConsole } from "../../../spec";
 
 import { ConsoleLogService } from "./console-log.service";
 
-let caughtMessage: any;
-
 describe("ConsoleLogService", () => {
+  const error = new Error("this is an error");
+  const obj = { a: 1, b: 2 };
+  let consoleSpy: {
+    log: jest.Mock<any, any>;
+    warn: jest.Mock<any, any>;
+    error: jest.Mock<any, any>;
+  };
   let logService: ConsoleLogService;
+
   beforeEach(() => {
-    caughtMessage = {};
-    interceptConsole(caughtMessage);
+    consoleSpy = interceptConsole();
     logService = new ConsoleLogService(true);
   });
 
@@ -18,41 +23,41 @@ describe("ConsoleLogService", () => {
 
   it("filters messages below the set threshold", () => {
     logService = new ConsoleLogService(true, () => true);
-    logService.debug("debug");
-    logService.info("info");
-    logService.warning("warning");
-    logService.error("error");
+    logService.debug("debug", error, obj);
+    logService.info("info", error, obj);
+    logService.warning("warning", error, obj);
+    logService.error("error", error, obj);
 
-    expect(caughtMessage).toEqual({});
+    expect(consoleSpy.log).not.toHaveBeenCalled();
+    expect(consoleSpy.warn).not.toHaveBeenCalled();
+    expect(consoleSpy.error).not.toHaveBeenCalled();
   });
+
   it("only writes debug messages in dev mode", () => {
     logService = new ConsoleLogService(false);
 
     logService.debug("debug message");
-    expect(caughtMessage.log).toBeUndefined();
+    expect(consoleSpy.log).not.toHaveBeenCalled();
   });
 
   it("writes debug/info messages to console.log", () => {
-    logService.debug("this is a debug message");
-    expect(caughtMessage).toMatchObject({
-      log: { "0": "this is a debug message" },
-    });
+    logService.debug("this is a debug message", error, obj);
+    logService.info("this is an info message", error, obj);
 
-    logService.info("this is an info message");
-    expect(caughtMessage).toMatchObject({
-      log: { "0": "this is an info message" },
-    });
+    expect(consoleSpy.log).toHaveBeenCalledTimes(2);
+    expect(consoleSpy.log).toHaveBeenCalledWith("this is a debug message", error, obj);
+    expect(consoleSpy.log).toHaveBeenCalledWith("this is an info message", error, obj);
   });
+
   it("writes warning messages to console.warn", () => {
-    logService.warning("this is a warning message");
-    expect(caughtMessage).toMatchObject({
-      warn: { 0: "this is a warning message" },
-    });
+    logService.warning("this is a warning message", error, obj);
+
+    expect(consoleSpy.warn).toHaveBeenCalledWith("this is a warning message", error, obj);
   });
+
   it("writes error messages to console.error", () => {
-    logService.error("this is an error message");
-    expect(caughtMessage).toMatchObject({
-      error: { 0: "this is an error message" },
-    });
+    logService.error("this is an error message", error, obj);
+
+    expect(consoleSpy.error).toHaveBeenCalledWith("this is an error message", error, obj);
   });
 });

libs/common/src/platform/services/console-log.service.ts

@@ -9,26 +9,26 @@ export class ConsoleLogService implements LogServiceAbstraction {
     protected filter: (level: LogLevelType) => boolean = null,
   ) {}
 
-  debug(message: string) {
+  debug(message?: any, ...optionalParams: any[]) {
     if (!this.isDev) {
       return;
     }
-    this.write(LogLevelType.Debug, message);
+    this.write(LogLevelType.Debug, message, ...optionalParams);
   }
 
-  info(message: string) {
-    this.write(LogLevelType.Info, message);
+  info(message?: any, ...optionalParams: any[]) {
+    this.write(LogLevelType.Info, message, ...optionalParams);
   }
 
-  warning(message: string) {
-    this.write(LogLevelType.Warning, message);
+  warning(message?: any, ...optionalParams: any[]) {
+    this.write(LogLevelType.Warning, message, ...optionalParams);
   }
 
-  error(message: string) {
-    this.write(LogLevelType.Error, message);
+  error(message?: any, ...optionalParams: any[]) {
+    this.write(LogLevelType.Error, message, ...optionalParams);
   }
 
-  write(level: LogLevelType, message: string) {
+  write(level: LogLevelType, message?: any, ...optionalParams: any[]) {
     if (this.filter != null && this.filter(level)) {
       return;
     }
@@ -36,19 +36,19 @@ export class ConsoleLogService implements LogServiceAbstraction {
     switch (level) {
       case LogLevelType.Debug:
         // eslint-disable-next-line
-        console.log(message);
+        console.log(message, ...optionalParams);
         break;
       case LogLevelType.Info:
         // eslint-disable-next-line
-        console.log(message);
+        console.log(message, ...optionalParams);
         break;
       case LogLevelType.Warning:
         // eslint-disable-next-line
-        console.warn(message);
+        console.warn(message, ...optionalParams);
         break;
       case LogLevelType.Error:
         // eslint-disable-next-line
-        console.error(message);
+        console.error(message, ...optionalParams);
         break;
       default:
         break;