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

Conversation

Saurabhkmr98
Copy link
Collaborator

@Saurabhkmr98 Saurabhkmr98 commented Dec 11, 2024

Description

Created a node package for logger as a wrapper around winston with configurable log level. This can be used for any node server side application.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Improvement (change that would cause existing functionality to not work as expected)
  • Code refactoring
  • Performance improvements
  • Documentation update

Screenshots and Media (if applicable)

Screenshot 2024-12-11 at 20 37 34

Test Scenarios

Imported the package in a server side application and tested the logs

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a logging package (@plane/logger) with a singleton-based logging utility.
    • Added middleware for logging incoming HTTP requests in Express applications.
    • Enhanced module exports to include configurations and middleware functionalities.
  • Documentation

    • Added a comprehensive README.md for the Logger Package detailing usage and features.
  • Configuration

    • Introduced new configuration files for ESLint, Prettier, and TypeScript, ensuring consistent code quality and formatting.
  • Bug Fixes

    • Updated linting settings to ignore generated directories for cleaner output.

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

coderabbitai bot commented Dec 11, 2024

Walkthrough

This pull request introduces several new files and configurations for the @plane/logger package. Key additions include a new ESLint configuration file, a Prettier configuration file, a README file detailing the logging utility, and a package.json file specifying package metadata. Additionally, TypeScript configuration and two new source files (config.ts and middleware.ts) are added, enhancing logging capabilities and middleware functionality. The changes ensure that generated files are ignored during linting, provide a structured logging configuration, and facilitate logging of HTTP requests in Express applications.

Changes

File Path Change Summary
packages/logger/.eslintignore Added entries to ignore build, dist, and out directories during linting.
packages/logger/.eslintrc.js Introduced ESLint configuration file, extending from @plane/eslint-config/library.js.
packages/logger/.prettierrc Added Prettier configuration file with specified formatting options.
packages/logger/README.md Created README file detailing the Logger Package's features and usage examples.
packages/logger/package.json Introduced package.json file with metadata, scripts, and dependencies for the logger package.
packages/logger/src/index.ts Updated to export all entities from config and middleware modules.
packages/logger/tsconfig.json Added TypeScript configuration file extending from @plane/typescript-config/base.json.
packages/logger/src/config.ts Introduced logging configuration using winston, defining custom log levels and transports.
packages/logger/src/middleware.ts Added requestLogger middleware function for logging HTTP requests in Express applications.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Server
    participant Logger

    Client->>Server: Sends HTTP request
    Server->>Logger: requestLogger(req)
    Logger-->>Server: Log request details
    Server->>Client: Sends response
    Logger-->>Logger: Log response status and duration
Loading

🐰 "In the logger's cozy nook,
With ESLint and Prettier, we took a look.
Configs and middleware, all in line,
Logging requests, oh how divine!
A README to guide, with features galore,
Hopping through code, we celebrate more!" 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between df66e92 and 739e15f.

📒 Files selected for processing (1)
  • packages/logger/src/config.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/logger/src/config.ts

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Experiment)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (5)
packages/logger/README.md (2)

1-4: Enhance the introduction with installation instructions

The README would benefit from a dedicated "Installation" section before the "Usage" section, detailing:

  • npm/yarn installation commands
  • Peer dependencies (if any)
  • Node.js version requirements

12-20: Fix package.json example syntax

The package.json example contains a syntax error and could be more complete.

Consider updating to:

{
  "dependencies": {
    "@plane/logger": "*"
  }
}
packages/logger/src/client-logger.ts (1)

31-36: Enhance error handling for transports

Currently, transport errors are logged to the console. Depending on the application's requirements, consider implementing additional error handling mechanisms such as alerting, retries, or fallback strategies to ensure robust logging.

packages/logger/src/server-logger.ts (1)

14-17: Avoid using synchronous file system methods in a server environment

Using fs.existsSync and fs.mkdirSync blocks the event loop, which can impact performance. Consider using asynchronous methods to prevent blocking.

Apply this diff to use asynchronous methods:

-if (!fs.existsSync(logDirectory)) {
-  fs.mkdirSync(logDirectory);
-  console.log('Logs folder created');
-}
+fs.access(logDirectory, fs.constants.F_OK, (err) => {
+  if (err) {
+    fs.mkdir(logDirectory, { recursive: true }, (err) => {
+      if (err) {
+        console.error('Error creating logs folder:', err);
+      } else {
+        console.log('Logs folder created');
+      }
+    });
+  }
+});
packages/logger/src/logger.ts (1)

17-17: Remove debugging console.log statement

The console.log("inside client logger import") statement appears to be for debugging purposes. It may be unnecessary in production code and could clutter the console output.

Apply this diff to remove the debug statement:

-  console.log("inside client logger import");
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 38e8a5c and 821a994.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (11)
  • packages/logger/.eslintignore (1 hunks)
  • packages/logger/.eslintrc.js (1 hunks)
  • packages/logger/.prettierrc (1 hunks)
  • packages/logger/README.md (1 hunks)
  • packages/logger/package.json (1 hunks)
  • packages/logger/src/client-logger.ts (1 hunks)
  • packages/logger/src/index.ts (1 hunks)
  • packages/logger/src/logger.ts (1 hunks)
  • packages/logger/src/server-logger.ts (1 hunks)
  • packages/logger/tsconfig.json (1 hunks)
  • web/package.json (1 hunks)
✅ Files skipped from review due to trivial changes (6)
  • packages/logger/src/index.ts
  • packages/logger/.eslintignore
  • packages/logger/.eslintrc.js
  • packages/logger/.prettierrc
  • packages/logger/tsconfig.json
  • packages/logger/package.json
🧰 Additional context used
🪛 LanguageTool
packages/logger/README.md

[uncategorized] ~27-~27: Loose punctuation mark.
Context: ... from "@plane/logger"; ``` ### logger: General Logger Use this for general app...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~48-~48: You might be missing the article “an” here.
Context: ...g the first import of logger by passing optional logLevel param in getLogger function.

(AI_EN_LECTOR_MISSING_DETERMINER_AN)

🔇 Additional comments (1)
web/package.json (1)

37-37: Consider using explicit versioning for production dependencies

While using "*" for version matching is common for internal packages, it's worth verifying if this aligns with the versioning strategy used for other @plane/* dependencies in this file.

✅ Verification successful

Versioning strategy is consistent across internal @plane packages

The use of "*" version for @plane/logger aligns with the versioning strategy used for all other internal @plane/* dependencies in the package.json file. This appears to be an intentional pattern for managing internal package dependencies within the monorepo.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check versioning pattern of other @plane/* dependencies
rg '"@plane/[^"]+": "[^"]+"' web/package.json

Length of output: 269

packages/logger/README.md Outdated Show resolved Hide resolved
packages/logger/README.md Outdated Show resolved Hide resolved
Comment on lines 40 to 45
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.

Comment on lines 74 to 79
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.

Comment on lines 15 to 22
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.

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.

"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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (4)
packages/logger/README.md (2)

27-27: Fix Markdown formatting for the heading

There is no blank line between the code block and the heading at line 27, which may cause rendering issues. Please add a blank line between the code block and the heading.

Apply this diff:

 }
+```

 ### `logger`: General Logger
 Use this for general application logs.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~27-~27: Loose punctuation mark.
Context: ... from "@plane/logger"; ``` ### logger: General Logger Use this for general app...

(UNLIKELY_OPENING_PUNCTUATION)


58-58: Add missing article "an"

Line 58 is missing the article "an" before "optional". It should read "by passing an optional logLevel param".

Apply this diff:

- You can specify a log level during the first import of logger by passing optional logLevel param in getLogger function.
+ You can specify a log level during the first import of the logger by passing an optional `logLevel` param in the `getLogger` function.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~58-~58: You might be missing the article “an” here.
Context: ...g the first import of logger by passing optional logLevel param in getLogger function.

(AI_EN_LECTOR_MISSING_DETERMINER_AN)

packages/logger/src/server-logger.ts (1)

15-18: Avoid synchronous file system operations

Using synchronous file system methods like fs.existsSync and fs.mkdirSync can block the event loop and impact performance. Consider using asynchronous methods instead.

Apply this diff:

-if (!fs.existsSync(logDirectory)) {
-  fs.mkdirSync(logDirectory);
-  console.log('Logs folder created');
-}
+fs.promises.mkdir(logDirectory, { recursive: true })
+  .then(() => console.log('Logs folder created'))
+  .catch((err) => {
+    if (err.code !== 'EEXIST') {
+      console.error('Error creating logs folder:', err);
+    }
+  });

Ensure that any code depending on the existence of the log directory accounts for the asynchronous nature of this operation.

packages/logger/tsconfig.json (1)

4-6: Consider using a more conservative target for better compatibility.

While using ESNext for both module and target provides access to the latest features, it might limit compatibility with older Node.js versions. Consider using explicit versions (e.g., ES2020) based on your minimum supported Node.js version.

-    "module": "ESNext",
-    "target": "ESNext",
+    "module": "ES2020",
+    "target": "ES2020",
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 821a994 and df0fc65.

📒 Files selected for processing (8)
  • packages/logger/README.md (1 hunks)
  • packages/logger/index.d.ts (1 hunks)
  • packages/logger/package.json (1 hunks)
  • packages/logger/src/client-logger.ts (1 hunks)
  • packages/logger/src/index.ts (1 hunks)
  • packages/logger/src/logger.ts (1 hunks)
  • packages/logger/src/server-logger.ts (1 hunks)
  • packages/logger/tsconfig.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/logger/src/logger.ts
  • packages/logger/src/index.ts
  • packages/logger/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/logger/index.d.ts

[error] 6-6: ';' expected'

An explicit or implicit semicolon is expected here...

(parse)


[error] 7-7: ';' expected'

An explicit or implicit semicolon is expected here...

(parse)

packages/logger/src/client-logger.ts

[error] 28-28: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🪛 LanguageTool
packages/logger/README.md

[uncategorized] ~27-~27: Loose punctuation mark.
Context: ... from "@plane/logger"; ``` ### logger: General Logger Use this for general app...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~58-~58: You might be missing the article “an” here.
Context: ...g the first import of logger by passing optional logLevel param in getLogger function.

(AI_EN_LECTOR_MISSING_DETERMINER_AN)

🔇 Additional comments (6)
packages/logger/src/client-logger.ts (1)

20-25: ⚠️ Potential issue

Singleton pattern ignores subsequent logLevel changes

The getInstance method initializes the logger only once. Subsequent calls to getLogger with different logLevel values 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.

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

 public static getLogger(loggerOptions?: ILoggerOptions): IClientLogMethods {
-    const instance = ClientLogger.getInstance(loggerOptions);
+    const instance = ClientLogger.getInstance();
+    if (loggerOptions?.logLevel && loggerOptions.logLevel !== instance.logLevel) {
+        instance.logLevel = loggerOptions.logLevel;
+    }
     return instance.logMethods;
 }

Also applies to: 27-30

packages/logger/README.md (1)

56-58: Consider adding production configuration guidelines

Including guidance on production configurations can help users set up the logger effectively in different environments. This might cover best practices, log rotation settings, environment-specific configurations, and performance considerations.

Would you like assistance in providing detailed examples of production configuration guidelines?

🧰 Tools
🪛 LanguageTool

[uncategorized] ~58-~58: You might be missing the article “an” here.
Context: ...g the first import of logger by passing optional logLevel param in getLogger function.

(AI_EN_LECTOR_MISSING_DETERMINER_AN)

packages/logger/src/server-logger.ts (1)

75-80: ⚠️ Potential issue

Singleton pattern ignores subsequent loggerOptions changes

The getInstance method initializes the logger only once. Subsequent calls to getLogger with different loggerOptions will not update the existing logger instance. This may lead to unexpected behavior if different logLevel or logFilePrefix values are specified later.

Consider modifying the implementation to update the logger's configuration if new loggerOptions are provided.

Apply this diff to update the logger's configuration dynamically:

 private static getInstance() {
     if (!Logger.instance) {
         Logger.instance = new Logger();
     }
     return Logger.instance;
 }

 public static getLogger(loggerOptions?: ILoggerOptions) {
     const instance = Logger.getInstance();
+    if (loggerOptions?.logLevel && loggerOptions.logLevel !== instance.logLevel) {
+        instance.logger.level = loggerOptions.logLevel;
+        instance.logLevel = loggerOptions.logLevel;
+    }
+    if (loggerOptions?.logFilePrefix && loggerOptions.logFilePrefix !== instance.logFilePrefix) {
+        // Logic to update the logFilePrefix
+        instance.logFilePrefix = loggerOptions.logFilePrefix;
+        // Reconfigure transports as necessary
+    }
     return instance.logger;
 }

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

packages/logger/tsconfig.json (3)

17-18: LGTM! Source file patterns are properly configured.

The include/exclude patterns are well-defined, including all source files while properly excluding node_modules.


2-2: Verify the base TypeScript configuration.

Let's ensure the base configuration file exists and is properly set up.

✅ Verification successful

Base TypeScript configuration is properly set up and accessible

The base configuration exists at packages/typescript-config/base.json and provides a solid foundation with:

  • Modern module resolution (NodeNext)
  • Strict type checking
  • Declaration file generation
  • Essential compiler safeguards

The configuration is properly published as part of the @plane/typescript-config package and the logger's tsconfig.json correctly extends it.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence and content of the base TypeScript configuration

# Check if the base config exists
fd -t f "base.json" -p "@plane/typescript-config"

# If found, examine its content
fd -t f "base.json" -p "@plane/typescript-config" -x cat {}

Length of output: 331


Script:

#!/bin/bash
# Let's try different approaches to locate the TypeScript configuration
# 1. Find all tsconfig files to understand the structure
fd tsconfig

# 2. Look for any package.json that might reference typescript-config
fd "package.json" -x grep -l "typescript-config"

# 3. Search for base.json in the entire repository
fd "base.json"

Length of output: 494


Script:

#!/bin/bash
# Let's examine the base.json content and verify it's properly configured
cat packages/typescript-config/base.json

# Also check the package.json of typescript-config to understand its setup
cat packages/typescript-config/package.json

Length of output: 801


11-13: Verify path alias configuration in build setup.

The path alias @/* needs corresponding configuration in your build tools (e.g., webpack, rollup, etc.) to work correctly.

✅ Verification successful

Let me gather more information about the build setup to verify the path alias configuration.


Let me check the tsup configuration since it's being used as the build tool.


Path alias configuration is properly handled by tsup

The project uses tsup as the build tool, which automatically handles TypeScript path aliases from tsconfig.json during the build process. No additional build configuration is needed as tsup inherently supports TypeScript's path mapping feature.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for build configuration files that might need path alias setup

# Search for common build config files
fd -t f "webpack.config|rollup.config|vite.config|package.json" -p "packages/logger"

# Check if path aliases are configured in package.json
fd -t f "package.json" -p "packages/logger" -x jq '.exports' {}

Length of output: 390


Script:

#!/bin/bash
# Check for build configuration in package.json and any build scripts
fd -t f "package.json" -p "packages/logger" -x jq '.scripts' {}

# Look for tsconfig references in the project
fd -t f "tsconfig" -p "packages/logger"

# Check if there's any build tool configuration
fd -t f "rollup|webpack|vite|babel|esbuild" -p "packages/logger"

Length of output: 373


Script:

#!/bin/bash
# Check if there's a tsup config file
fd -t f "tsup.config" -p "packages/logger"

# Look at the actual source files to see if path aliases are being used
rg -g "*.{ts,tsx}" "from '@/" packages/logger/src

Length of output: 95

}

public static getLogger(loggerOptions?: ILoggerOptions): IClientLogMethods {
const instance = this.getInstance(loggerOptions);
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

Use class name instead of 'this' in static method

In static methods, using this can be confusing because it refers to the class itself. Replace this with the class name ClientLogger for clarity.

Apply this diff:

 public static getLogger(loggerOptions?: ILoggerOptions): IClientLogMethods {
-    const instance = this.getInstance(loggerOptions);
+    const instance = ClientLogger.getInstance(loggerOptions);
     return instance.logMethods;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const instance = this.getInstance(loggerOptions);
const instance = ClientLogger.getInstance(loggerOptions);
🧰 Tools
🪛 Biome (1.9.4)

[error] 28-28: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

packages/logger/index.d.ts Outdated Show resolved Hide resolved
packages/logger/index.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (5)
packages/logger/src/config.ts (4)

5-12: Add JSDoc comments for log levels

Consider adding JSDoc comments to document the purpose and usage of each log level. This would help other developers understand when to use each level appropriately.

 // Define log levels
+/**
+ * Log levels in order of increasing verbosity:
+ * - error: For errors that need immediate attention
+ * - warn: For potentially harmful situations
+ * - info: For general information about system operation
+ * - http: For HTTP request logging
+ * - debug: For detailed debugging information
+ */
 const levels = {

14-24: Add TypeScript type definitions for colors

Consider adding a type definition for the colors object to improve type safety.

+type LogColors = {
+  [K in keyof typeof levels]: string;
+};
+
 // Define colors for each level
-const colors = {
+const colors: LogColors = {
   error: "red",
   warn: "yellow",
   info: "green",
   http: "magenta",
   debug: "white",
 };

26-33: Consider enhancing the log format

The current format is clean but could be more informative for debugging purposes. Also, colorizing all content might interfere with log parsing tools.

 const format = winston.format.combine(
   winston.format.timestamp({ format: "YYYY-MM-DD HH:mm:ss:ms" }),
-  winston.format.colorize({ all: true }),
+  winston.format.colorize({ level: true }),
   winston.format.printf(
-    (info: winston.Logform.TransformableInfo) => `[${info?.timestamp}] ${info.level}: ${info.message}`
+    (info: winston.Logform.TransformableInfo) => `[${info?.timestamp}] ${info.level}: ${info.message}${info.stack ? '\n' + info.stack : ''}${
+      Object.keys(info).some(key => !['timestamp', 'level', 'message', 'stack'].includes(key))
+        ? '\n' + JSON.stringify(Object.assign({}, info, { timestamp: undefined, level: undefined, message: undefined, stack: undefined }))
+        : ''
+    }`
   )
 );

1-66: Add tests and usage documentation

Consider adding:

  1. Unit tests for logger configuration
  2. Integration tests for file rotation
  3. Usage examples in the README
  4. Documentation for available environment variables

This is a critical infrastructure component that needs thorough testing and documentation.

Would you like me to help create:

  1. A test suite template?
  2. Usage examples for the README?
  3. Environment variables documentation?
packages/logger/src/middleware.ts (1)

1-2: Consider adding type safety for the logger interface

While the Express types are properly imported, we should ensure type safety for the logger interface to prevent potential runtime errors if the logger implementation changes.

 import { Request, Response, NextFunction } from "express";
+import type { Logger } from 'winston';
 import { logger } from "./config";
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between df0fc65 and c9d0a02.

📒 Files selected for processing (4)
  • packages/logger/package.json (1 hunks)
  • packages/logger/src/config.ts (1 hunks)
  • packages/logger/src/index.ts (1 hunks)
  • packages/logger/src/middleware.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/logger/package.json
  • packages/logger/src/index.ts
🔇 Additional comments (2)
packages/logger/src/config.ts (1)

1-3: Verify package dependencies and add type definitions

Please ensure these dependencies are properly listed in package.json. Also, consider adding type definitions for winston-daily-rotate-file.

packages/logger/src/middleware.ts (1)

4-7: 🛠️ Refactor suggestion

Add error handling wrapper

The middleware should be wrapped in a try-catch block to prevent unhandled exceptions from crashing the application.

 export const requestLogger = (req: Request, res: Response, next: NextFunction) => {
+  try {
     // Log when the request starts
     const startTime = Date.now();
+  } catch (error) {
+    logger.error('Error in request logger middleware:', error);
+    next(error);
+  }

Likely invalid or redundant comment.

packages/logger/src/config.ts Show resolved Hide resolved
packages/logger/src/config.ts Outdated Show resolved Hide resolved
packages/logger/src/middleware.ts Show resolved Hide resolved
packages/logger/src/middleware.ts Show resolved Hide resolved
packages/logger/src/middleware.ts Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)
packages/logger/README.md (3)

5-5: Remove trailing period from heading

Remove the trailing period from the "Features" heading to follow markdown conventions.

-## Features.
+## Features
🧰 Tools
🪛 Markdownlint (0.37.0)

5-5: Punctuation: '.'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


3-3: Enhance the introduction with structured logging details

Consider expanding the introduction to highlight the structured logging capabilities, including:

  • JSON log format
  • Correlation ID support
  • Log metadata enrichment

55-55: Fix hyphenation in rotation period

Add a hyphen to the compound adjective.

-Log files have a 7 day rotation period defined.
+Log files have a 7-day rotation period defined.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~55-~55: When a number forms part of an adjectival compound, use a hyphen.
Context: ...mbined-%DATE%.log`. - Log files have a 7 day rotation period defined. ## Configurat...

(MISSING_HYPHEN)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9d0a02 and df66e92.

📒 Files selected for processing (1)
  • packages/logger/README.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/logger/README.md

[uncategorized] ~36-~36: Loose punctuation mark.
Context: ... is an error"); ``` ### requestLogger: Request Logger Middleware Use this as a...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~55-~55: When a number forms part of an adjectival compound, use a hyphen.
Context: ...mbined-%DATE%.log`. - Log files have a 7 day rotation period defined. ## Configurat...

(MISSING_HYPHEN)

🪛 Markdownlint (0.37.0)
packages/logger/README.md

5-5: Punctuation: '.'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

🔇 Additional comments (3)
packages/logger/README.md (3)

30-34: Add error handling examples and type information

The current examples are too basic. They should demonstrate:

  1. Error handling with proper typing
  2. Structured logging with metadata
  3. Async logging scenarios

57-59: Add production configuration guidance

The configuration section should include production best practices, log rotation settings, and environment-specific configurations.


7-8: Verify logger implementation matches documentation

Let's verify that the actual implementation supports all documented features.

✅ Verification successful

Let me gather more information about the winston logger configuration since the first pattern didn't show the complete implementation.


Let me check the complete content of the logger configuration file to verify all the features.


Documentation accurately reflects the logger implementation

The implementation fully supports the documented features:

  • Pre-configured winston logger is properly implemented with comprehensive configuration including:
    • Multiple log levels (error, warn, info, http, debug)
    • Console and rotating file transports
    • Configurable log level via environment variable
  • Request logger middleware is implemented with detailed request/response logging capabilities
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify logger implementation matches documentation

# Check if winston logger is properly configured
ast-grep --pattern 'import { createLogger } from "winston"'

# Verify log level configuration from env
ast-grep --pattern 'process.env.LOG_LEVEL'

# Check request logger middleware implementation
ast-grep --pattern $'export const requestLogger = (req: Request, res: Response, next: NextFunction) => {
  $$$
}'

Length of output: 1623


Script:

#!/bin/bash
# Check the complete winston logger configuration
ast-grep --pattern $'export const logger = createLogger({
  $$$
})'

# Also check if there's a general usage export
rg -l "export.*logger" packages/logger/src/

Length of output: 144


Script:

#!/bin/bash
# Get the complete logger configuration
cat packages/logger/src/config.ts

Length of output: 1521

packages/logger/README.md Show resolved Hide resolved
@sriramveeraghanta sriramveeraghanta merged commit a8140a5 into preview Dec 13, 2024
12 of 13 checks passed
@sriramveeraghanta sriramveeraghanta deleted the chore-logger-package branch December 13, 2024 09:03
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

Successfully merging this pull request may close these issues.

3 participants