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

chore: Add logger package for node server side apps #6188

Merged
merged 9 commits into from
Dec 13, 2024
3 changes: 3 additions & 0 deletions packages/logger/.eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
build/*
dist/*
out/*
9 changes: 9 additions & 0 deletions packages/logger/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/** @type {import("eslint").Linter.Config} */
module.exports = {
root: true,
extends: ["@plane/eslint-config/library.js"],
parser: "@typescript-eslint/parser",
parserOptions: {
project: true,
},
};
5 changes: 5 additions & 0 deletions packages/logger/.prettierrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"printWidth": 120,
"tabWidth": 2,
"trailingComma": "es5"
}
48 changes: 48 additions & 0 deletions packages/logger/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Logger Package

This package provides a singleton-based logger utility built using [Winston](https://github.com/winstonjs/winston). It offers customizable log levels and supports structured logging for general application logs and HTTP requests.

## Features
- Singleton pattern ensures a single logger instance.
- Dynamic log level configuration and log filename prefix.
- Pre-configured winston logger for general usage (`logger`).

## Usage

### Adding as a package
Add this package as a dependency in package.json
```typescript
dependency: {
...
@plane/logger":"*",
...
}
sriramveeraghanta marked this conversation as resolved.
Show resolved Hide resolved
```

### Importing the Logger
```typescript
import PlaneLogger from "@plane/logger";
```

### `logger`: General Logger
Use this for general application logs.

```typescript
const logger: Logger = PlaneLogger.getLogger("info", "log-file-prefix")
logger.info("This is an info log");
logger.warn("This is a warning");
logger.error("This is an error");
```
sriramveeraghanta marked this conversation as resolved.
Show resolved Hide resolved

## Available Log Levels
- `error`
- `warn`
- `info` (default)
- `http`
- `verbose`
- `debug`
- `silly`

## Configuration
- By default, the log level is set to `info`.
- You can specify a log level during the first import of logger by passing optional logLevel param in getLogger function.
sriramveeraghanta marked this conversation as resolved.
Show resolved Hide resolved
29 changes: 29 additions & 0 deletions packages/logger/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"name": "@plane/logger",
"version": "1.0.0",
"description": "Logger shared across multiple apps internally",
"private": true,
"main": "./dist/index.js",
"type":"module",
"module": "./dist/index.mjs",
"types": "./dist/index.d.ts",
"files": [
"dist/**"
],
"scripts": {
"build": "tsup ./src/index.ts --format esm,cjs --dts --external react --minify",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

donot create a build for this package. You can directly import them and use it. Let the application that is using it transpile these packages.

"lint": "eslint src --ext .ts,.tsx",
"lint:errors": "eslint src --ext .ts,.tsx --quiet"
},
"dependencies": {
"winston": "^3.17.0",
"winston-daily-rotate-file": "^5.0.0"
},
"devDependencies": {
"@plane/eslint-config": "*",
"@types/node": "^22.5.4",
"@types/react": "^18.3.11",
"tsup": "^7.2.0",
"typescript": "^5.3.3"
}
}
53 changes: 53 additions & 0 deletions packages/logger/src/client-logger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { createLogger, format, transports, Logger as WinstonLogger } from "winston";


export default class ClientLogger {
private static instance: ClientLogger;
private logger: WinstonLogger;
private logLevel: string;

private constructor(logLevel: string = "info") {

this.logLevel = logLevel;

this.logger = createLogger({
level: this.logLevel,
format: format.combine(
format.colorize(),
format.timestamp({
format: "DD/MMM/YYYY HH:mm:ss",
}),
format.printf(({ timestamp, level, message, ...metadata }) => {
const msg = `[${timestamp}] "${level}" ${message}`;
const metaString = Object.keys(metadata).length ? ` ${JSON.stringify(metadata)}` : "";
return msg + metaString;
})
),
transports: [
new transports.Console({ handleExceptions: true })
],
});

this.logger.transports.forEach((transport) => {
transport.on("error", (err) => {
// Handle the error, log it, or notify as necessary
console.error(`Logging transport error: Console`, err);
});
});

}

private static getInstance(logLevel?: string) {
if (!ClientLogger.instance) {
ClientLogger.instance = new ClientLogger(logLevel);
}
return ClientLogger.instance;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Singleton pattern may ignore subsequent logLevel changes

The getInstance method initializes the logger only once. Subsequent calls to getLogger with a different logLevel will not update the log level of the existing logger instance, which may lead to unexpected behavior if different log levels are specified later.

Consider modifying the implementation to allow updating the log level dynamically or document that the log level is set only during the first initialization.

Apply this diff to update the logger's level if a new logLevel is provided:

 public static getLogger(logLevel?: string) {
-    const instance = ClientLogger.getInstance(logLevel);
+    const instance = ClientLogger.getInstance();
+    if (logLevel && logLevel !== instance.logLevel) {
+        instance.logger.level = logLevel;
+        instance.logLevel = logLevel;
+    }
     return instance.logger;
 }

Committable suggestion skipped: line range outside the PR's diff.


public static getLogger(logLevel?: string) {
const instance = ClientLogger.getInstance(logLevel);
return instance.logger
}

}

2 changes: 2 additions & 0 deletions packages/logger/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// src/index.ts
export { default } from './server-logger';
24 changes: 24 additions & 0 deletions packages/logger/src/logger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { Logger as WinstonLogger } from 'winston';

export interface ILogger {
logger: WinstonLogger; // The Winston logger instance
logLevel?: string; // The current logging level
logFilePrefix?: string;

// Method to get the logger instance
getLogger(logLevel?: string, logFilePrefix?: string): WinstonLogger;
}


let Logger: ILogger;

if (typeof window !== "undefined") {
// Client-side logic
console.log("inside client logger import")
Logger = require('./client-logger').default;
} else {
// Server-side logic
Logger = require('./server-logger').default;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Mixing ES modules and CommonJS require statements

The code uses require statements in a module that uses import statements. This can lead to compatibility issues, especially when using TypeScript with ES module syntax. Consider using dynamic import() for conditional imports.

Apply this diff to replace require with dynamic imports:

 let Logger: ILogger;

-if (typeof window !== "undefined") {
-  // Client-side logic
-  console.log("inside client logger import")
-  Logger = require('./client-logger').default;
-} else {
-  // Server-side logic
-  Logger = require('./server-logger').default;
-}
+if (typeof window !== "undefined") {
+  // Client-side logic
+  console.log("inside client logger import");
+  import('./client-logger').then((module) => {
+    Logger = module.default;
+  });
+} else {
+  // Server-side logic
+  import('./server-logger').then((module) => {
+    Logger = module.default;
+  });
+}

Alternatively, ensure that the module system is consistent throughout the project.

Committable suggestion skipped: line range outside the PR's diff.


export default Logger;
87 changes: 87 additions & 0 deletions packages/logger/src/server-logger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import { createLogger, format, transports, Logger as WinstonLogger } from "winston";
import winstonRotate from "winston-daily-rotate-file";
import { fileURLToPath } from 'url';
import { dirname } from 'path';
import fs from 'fs';

// Get current directory
const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);

// Set up log directory
const logDirectory = `${__dirname}/logs`;

if (!fs.existsSync(logDirectory)) {
fs.mkdirSync(logDirectory);
console.log('Logs folder created');
}


export default class Logger {
private static instance: Logger;
private logger: WinstonLogger;
private logLevel: string;
private logFilePrefix: string;

private constructor(logLevel: string = "info", logFilePrefix: string = "plane-log") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convert those log level and log file prefix as entities inside an object.

Basically constructor will take a object, you destruct it and use it.

By doing this like and server logger can be initialised with same type of object.

this.logLevel = logLevel;
this.logFilePrefix = logFilePrefix;

this.logger = createLogger({
level: this.logLevel,
format: format.combine(
format.colorize(),
format.timestamp({
format: "DD/MMM/YYYY HH:mm:ss",
}),
format.printf(({ timestamp, level, message, ...metadata }) => {
const msg = `[${timestamp}] "${level}" ${message}`;
const metaString = Object.keys(metadata).length ? ` ${JSON.stringify(metadata)}` : "";
return msg + metaString;
})
),
transports: [
new winstonRotate({
filename: `${logDirectory}/${this.logFilePrefix}-%DATE%.log`,
datePattern: 'YYYY-MM-DD',
maxSize: '20m', // Optional: maximum size per log file
maxFiles: '7d', // Keep logs for 7 days
zippedArchive: true, // Optional: compress archived logs
}),
],
});

this.logger.transports.forEach((transport) => {
transport.on("error", (err) => {
// Handle the error, log it, or notify as necessary
const transportType: string = this.getTransportType(transport);
console.error(`Logging transport error: ${transportType}`, err);
});
});

}

private getTransportType(transport: any): string {
if (transport instanceof transports.Console) {
return "Console";
} else if (transport instanceof winstonRotate) {
return "File (Rotation)";
} else {
return "Unknown";
}
}

private static getInstance(logLevel?: string, logFilePrefix?: string) {
if (!Logger.instance) {
Logger.instance = new Logger(logLevel, logFilePrefix);
}
return Logger.instance;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Singleton pattern may ignore subsequent logLevel and logFilePrefix changes

The getInstance method initializes the logger once. Subsequent calls to getLogger with different logLevel or logFilePrefix will not update the existing logger instance, which may cause unexpected behavior if these parameters are changed later.

Consider modifying the implementation to handle dynamic updates to logLevel and logFilePrefix or document that these parameters are set only during the first initialization.

Apply this diff to update the logger's configuration if new parameters are provided:

 public static getLogger(logLevel?: string, logFilePrefix?: string) {
-    const instance = Logger.getInstance(logLevel, logFilePrefix);
+    const instance = Logger.getInstance();
+    if (logLevel && logLevel !== instance.logLevel) {
+        instance.logger.level = logLevel;
+        instance.logLevel = logLevel;
+    }
+    if (logFilePrefix && logFilePrefix !== instance.logFilePrefix) {
+        // Logic to update the logFilePrefix, which may require reconfiguring transports
+        instance.logFilePrefix = logFilePrefix;
+        // Reconfigure or recreate transports as necessary
+    }
     return instance.logger;
 }

Note: Updating the logFilePrefix may require recreating the transport to apply the new file naming convention.

Committable suggestion skipped: line range outside the PR's diff.


public static getLogger(logLevel?: string, logFilePrefix?: string) {
const instance = Logger.getInstance(logLevel, logFilePrefix);
return instance.logger
}

}

17 changes: 17 additions & 0 deletions packages/logger/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"extends": "@plane/typescript-config/base.json",
"compilerOptions": {
"module": "ESNext",
"moduleResolution": "bundler",
"outDir": "./dist",
"rootDir": "./src",
"baseUrl": ".",
"paths": {
"@/*": ["./src/*"]
},
"experimentalDecorators": true,
"sourceMap": true
},
"include": ["src/**/*"],
"exclude": ["node_modules"]
}
Loading
Loading