Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Add logging framework with documentation on how to write good log selectors #4730

Open
wayfarer3130 opened this issue Jan 23, 2025 · 3 comments
Assignees

Comments

@wayfarer3130
Copy link
Contributor

What feature or change would you like to see made?

A logging framework would allow much better debugging and managing of the logging output. There are parts of the UI such as hanging protocols which generate a lot of log details in order to be able to figure out how to write a hanging protocol, but most of the time it is undesired. The logging framework should also support redirecting the logs to server side capture, and should be light weight.

Some specific feature requirements:

  • Some way to turn on logs persistently so that startup can have appropriate logging
    • One option is url parameters, eg debug=ohif to turn on ohif debug level logging
    • Another option is browser storage, with top level methods to easily adjust the logging, eg in a console window: log.ohif='debug'
  • A hierarchical naming/level scheme, so that broad groups can be turned on/off, AND sub-groups customized, eg like log.cs3d='warn' to turn off cs3d and then log.cs3d.tools.annotation='debug' to turn on just the cs3d annotation tools
  • A way to store specified paths to specific back ends, perhaps like setLogOutput('usage', new ServerStore()) to have usage information sent to the server store
  • Support unit tests to allow testing to turn off specific log levels before/after the tests to avoid garbage in the logs
  • Have a simple reset to default logs levels
  • Have a dev environment log level default set which is different from production
  • Allow full use in build environments
  • Have a log capture mechanism to allow submitting bug reports with logs
  • Be integrated with CS3D logging and dcmjs logging

Why should we prioritize this feature?

This change would allow developers to include relevant logs to make it much easier to debug things when users have somethign go wrong, as well as to debug during development without getting an entire dump of information

@pieper
Copy link
Member

pieper commented Jan 23, 2025

Yes, I would very much like to see this feature. The issue came up for me recently that in dcmjs some validation errors and warnings are sent to the console and I didn't see any easy way to capture these as part of a report about the data being processed. The ability to capture a defined set of logging data to a variable would be a valuable feature for me.

Also I think it would be nice to have a URL parameter flag at the application level so you can start catching the logs right away without needing to go into the dev console.

@salkz
Copy link

salkz commented Jan 24, 2025

Great initiative, something that I wanted to bring attention to for a long time now. I even made a POC for myself during holidays when I had time for some creativity (I didn't want to just install another node package).

This is something resembling a version 0.1 of a lightweight logger that I've had lots of success with so far:

export type TConsoleMethod =
  | 'log'
  | 'info'
  | 'trace'
  | 'warn'
  | 'debug'
  | 'error';

const defaultName = '@ohif';
const defaultStyle = 'color: cyan; font-weight: bold;';
const defaultMethods: TConsoleMethod[] = [
  'log',
  'info',
  'trace',
  'warn',
  'debug',
  'error',
];

export class Logger {
  // A map of name: Logger
  private static instances: Record<string, Logger> = {};

  private readonly style: string;

  private name: string;
  private methods: TConsoleMethod[] = defaultMethods;

  constructor(
    name: string = defaultName,
    style: string = defaultStyle,
    methods: TConsoleMethod[] = defaultMethods
  ) {
    const existing = Logger.instances[name];
    if (existing) return existing;

    this.name = name;
    this.style = style;
    this.setMethods(methods);

    Logger.registerInstance(this);
  }

  // Register each instance by guid to contain the instance and its prefix
  private static registerInstance(instance: Logger) {
    Logger.instances[instance.getName()] = instance;
  }

  private static unregisterInstance(instance: Logger) {
    delete Logger.instances[instance.getName()];
  }

  private static getInstance(name: string) {
    const instance = Logger.getInstance(name);
    if (!instance) {
      throw new Error(`Logger with name ${name} does not exist.`);
    }

    return instance;
  }

  private static getInstancesByPrefix(prefix: string): Logger[] {
    const filteredNames = Object.keys(Logger.instances).filter((name) =>
      name.startsWith(prefix)
    );
    return filteredNames.map((name) => Logger.instances[name]);
  }

  public static cloneLogger(name: string, nameExtension: string) {
    return Logger.getInstance(name).clone(nameExtension);
  }

  public static setLoggerMethods(name: string, methods: TConsoleMethod[]) {
    Logger.getInstance(name).setMethods(methods);
  }

  public static setPrefixMethods(prefix: string, methods: TConsoleMethod[]) {
    Logger.getInstancesByPrefix(prefix).forEach((instance) =>
      instance.setMethods(methods)
    );
  }

  public log(...data: any[]) {}
  public info(...data: any[]) {}
  public trace(...data: any[]) {}
  public warn(...data: any[]) {}
  public debug(...data: any[]) {}
  public error(...data: any[]) {}

  // Get the current prefix
  public getName() {
    return this.name;
  }

  // Set a new prefix
  public setName(name: string) {
    Logger.unregisterInstance(this);
    this.name = name;
    Logger.registerInstance(this);
  }

  public getMethods() {
    return this.methods;
  }

  public setMethods(methods: TConsoleMethod[]) {
    this.methods = methods;

    // Restore the methods with the new methods
    methods.forEach((method) => {
      this[method] = console[method].bind(
        console,
        `%c${this.name}`,
        this.style
      );
    });

    // Silence the methods that are not in the new methods
    defaultMethods.forEach((method) => {
      if (!methods.includes(method)) {
        this[method] = () => {};
      }
    });
  }

  public nap(...methods: TConsoleMethod[]) {
    const toSilence = methods.length ? methods : defaultMethods;
    // Silence the methods that are not already silenced
    this.setMethods(
      this.methods.filter((method) => !toSilence.includes(method))
    );
  }

  public yap(...methods: TConsoleMethod[]) {
    const toRestore = methods.length ? methods : defaultMethods;
    const silenced = toRestore.filter(
      (method) => !this.methods.includes(method)
    );
    // Restore the methods that are not already in the current methods
    this.setMethods(this.methods.concat(silenced));
  }

  // Clone the current logger with a new name extension separated by '/',
  // eg clone('@ohif').clone('extension') will produce '@ohif/extension'
  public clone(nameExt: string) {
    return new Logger(`${this.name}/${nameExt}`, this.style, this.methods);
  }
}

export const defaultLogger = new Logger();
export default defaultLogger;

Excuse the comical function names nap and yap, I couldn't resist.

Example usage:

const logger = defaultLogger.clone('dynamics');
const otherLogger = new Logger('@other', 'color: red; font-weight: bold;');
const thirdLogger = otherLogger.clone('third');

logger.debug('initiating dynamics viewer route');
otherLogger.debug('initiating dynamics viewer route');
thirdLogger.debug('initiating dynamics viewer route');

Image

Now we can modify logging globally with setPrefixMethods, for example if we want to silence everything with @other:

const logger = defaultLogger.clone('dynamics');
const otherLogger = new Logger('@other', 'color: red; font-weight: bold;');
const thirdLogger = otherLogger.clone('third');

Logger.setPrefixMethods('@other', []);

logger.debug('initiating dynamics viewer route');
otherLogger.debug('initiating dynamics viewer route');
thirdLogger.debug('initiating dynamics viewer route');

Image

In case of OHIF, you could set the log levels via config on initialization, like so:

platform/app/public/config/default.json

...
  extensions: [],
  modes: [],
  logLevels: ['log', 'info', 'warn', 'error', 'debug'],
  customizationService: {},
  showStudyList: true
...
Logger.setPrefixMethods('@ohif', appConfig.logLevels);

And of course, you could import the logger into your own extension or mode, create a new class instance with your own "namespace" and it wouldn't be affected by OHIF's config unless you explicitly use the same logger or use the same configuration.

But of course, this needs adjustments to match the feature requirements but at the same time, it's kind of like an MVP without too big of a scope when implementing it and replacing it with the existing platform/core/src/log.js.

Feel free to use this or extend it but of course I welcome any other logger implementation as well since the current situation is not great.

@Gabsha
Copy link

Gabsha commented Jan 29, 2025

This is a great idea, I'm also interested in this feature 👍 several services logs to the console by default, it would be great to have a mechanism for selective logging, and remotely capturing logs from users not familiar with dev tools could prove very handy for remote debugging.

Looking forward to this feature's development!


Food for thought: probably too complex for an mvp, but Python defines an extendable LogRecord object (name, level, filepath, datetime, msg, etc.) and use Formatters to serialize records. Filters can be attached to logger instances too. The added flexibility (vs keeping everything as a string) could prove useful in the future (eg. create a ui component that display processing logs of some algorithm)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants