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

Enhanced Logging System with Winston Integration #69

Merged
merged 26 commits into from
Mar 17, 2024

Conversation

Sweetdevil144
Copy link
Contributor

@Sweetdevil144 Sweetdevil144 commented Feb 3, 2024

Fixes #21
Description:

This pull request introduces significant improvements to our application's logging system, leveraging the winston library for a more structured and effective logging approach:

  1. Logger Utility Introduction:

    • New Logger.js File: Implemented in the util directory, this file features a custom Logger class using winston, standardizing logging across our codebase.
    • Logger Class Capabilities: Offers four logging levels - debug, info, warn, and error - with log verbosity adapting to the environment (development or production).
  2. Refactoring in app.js:

    • Replaced Console Logging: Transitioned from console.log and console.error to the Logger class methods for consistent logging.
    • Enhanced Logging Details: Logs, particularly for errors, now include comprehensive information like stack traces and context for better debugging.
  3. Adaptive Log Level Management:

    • Environment-Based Log Levels: The Logger sets log levels dynamically based on NODE_ENV, ensuring appropriate verbosity in development and production modes.
  4. Log Formatting and Management:

    • Improved Readability: Logs are now colorized and formatted for clarity, thanks to winston's features.
    • Versatile Log Storage: Configured with multiple transports, our logger ensures effective log data capture and storage in both console and files.

@ChakshuGautam
Copy link
Owner

@Sweetdevil144 is this ready to review?

@Sweetdevil144
Copy link
Contributor Author

Yes, go ahead!!

@dhruv-1001
Copy link
Collaborator

Hey, @Sweetdevil144. As the project grows, there will be more files/classes. And we'll need to know which class is throwing out those logs. Can you also add that logger functionality to this PR as well?

@Sweetdevil144
Copy link
Contributor Author

Sweetdevil144 commented Feb 6, 2024

@dhruv-1001 I think I already did it by adding server/utils/logger.js class. If more files are added, one can simply import the class and add logger functionalities to the same!!
Hope that helps !! (:

@Sweetdevil144
Copy link
Contributor Author

Sweetdevil144 commented Feb 6, 2024

I will add the class function as you requested for it to be thrown too!!

@dhruv-1001
Copy link
Collaborator

@Sweetdevil144, any update on this?

@Sweetdevil144
Copy link
Contributor Author

Hey @dhruv-1001 , I already added class logging property to the Logger. Just a simple guide. User will add two comments in Logging. For example

Logger.error('app.js',`Error reading folder: ${err}`);

It is already available for review. Any changes are welcome.

@Sweetdevil144
Copy link
Contributor Author

Sweetdevil144 commented Feb 12, 2024

I think the code is ready to be merged. Views @dhruv-1001 and @ChakshuGautam ??

@dhruv-1001
Copy link
Collaborator

@Sweetdevil144, can you make the changes i've mentioned in review?
Which says that, file name should be passed in constructor to function. So that we don't have to pass that in every time we log anything.

@Sweetdevil144
Copy link
Contributor Author

Can you check the changes now @dhruv-1001 ? I've added the filename to logger class so that it's passed only once when using the Logger file. For example, If you want to log in app.js, we do :

import Logger from './util/logger';
const logger = new Logger('app.js');

@dhruv-1001
Copy link
Collaborator

@Sweetdevil144, that's great. Just update the logs everywhere according to this change, and test it out on your local. I'll then merge this PR

@Sweetdevil144
Copy link
Contributor Author

Hey @dhruv-1001 and @ChakshuGautam . Can you checkout this PR?

@dhruv-1001
Copy link
Collaborator

@Sweetdevil144, that's great. Just update the logs everywhere according to this change, and test it out on your local. I'll then merge this PR

Can you push these changes?

@Sweetdevil144
Copy link
Contributor Author

@Sweetdevil144, that's great. Just update the logs everywhere according to this change, and test it out on your local. I'll then merge this PR

Can you push these changes?

Pushed

@Sweetdevil144
Copy link
Contributor Author

I guess we should remove combined.log and all other log files by adding them in .gitignore

@Sweetdevil144
Copy link
Contributor Author

Sweetdevil144 commented Feb 28, 2024

@dhruv-1001 ready for review

One more doubt : Why don't we have jest configured for our codebase? We can prepare our jest.config.js as follow :

   module.exports = {
     transform: {
       '^.+\\.js$': 'babel-jest',
     },
   };

It is because I have been facing the following issue while running our test cases :
SyntaxError: Cannot use import statement outside a module, which indicates that Jest is trying to run our tests in a CommonJS environment, which does not support ES6 module syntax (import/export). To resolve this issue, we'll need to configure Jest to use Babel for transforming our code, which will allow it to understand ES6 modules.

@dhruv-1001
Copy link
Collaborator

dhruv-1001 commented Mar 8, 2024

Hey @Sweetdevil144, will be reviewing this today evening.
Edit: Will try today.

@dhruv-1001
Copy link
Collaborator

This LGTM. Merging this now. Moving forward we are migrating to nestjs. So, will be using logging class by nestjs.

@dhruv-1001 dhruv-1001 merged commit b472d6b into ChakshuGautam:master Mar 17, 2024
1 check passed
35C4n0r pushed a commit that referenced this pull request Jun 5, 2024
* Add logging

---------

Co-authored-by: Sweetdevil144 <Sweetdevil144@users.noreply.github.com>
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.

Add Proper Logging
3 participants