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

Structured logging #17

Merged
merged 10 commits into from
Jan 5, 2025
Merged

Structured logging #17

merged 10 commits into from
Jan 5, 2025

Conversation

mdellabitta
Copy link
Contributor

@mdellabitta mdellabitta commented Jan 4, 2025

Important

Introduce structured logging with winston and morgan, replacing console statements and updating tests accordingly.

  • Logging:
    • Introduce winston for structured logging in logger.ts.
    • Replace console with logger in ResponseHelper.ts, ThumbnailApi.ts, and app.ts.
    • Add morgan middleware for HTTP request logging in app.ts.
  • Dependencies:
    • Add winston and morgan to dependencies in package.json.
    • Add @types/morgan to devDependencies in package.json.
  • Error Handling:
    • Update sendError in ThumbnailApi.ts to use logger.
    • Add process event listeners in app.ts for uncaughtException and unhandledRejection to log errors and exit.
  • Tests:
    • Update ThumbnailApi.test.ts to silence logger during tests and verify error handling.

This description was created by Ellipsis for 9512583. It will automatically update as commits are pushed.

- switched to winston for logging
- added morgan for access logging
- made /health never 304
- uncaughtException handler
- unhandledRejection handler
- respawning crashed worker fix
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 1ac9796 in 42 seconds

More details
  • Looked at 327 lines of code in 6 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. test/unit/ThumbnailApi.test.ts:54
  • Draft comment:
    Consider adding assertions to verify that the logger outputs the expected error messages. This ensures that logging is functioning correctly and that important information is not lost.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces structured logging using Winston, replacing console logs. This is a good practice for better log management and analysis. However, the test cases for logging are not comprehensive. The sendError method in ThumbnailApi is tested, but the logger's output is not verified. This could lead to missing log entries if the logger configuration changes or if there are issues in the logging logic.
2. test/unit/ThumbnailApi.test.ts:326
  • Draft comment:
    Consider adding assertions to verify that the logger outputs the expected error messages. This ensures that logging is functioning correctly and that important information is not lost. This comment applies to other test cases where errors are logged, such as proxyItemFromContributor: bad body and upstream failed.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new logger using Winston, but the test cases do not verify the logger's output. This could lead to missing log entries if the logger configuration changes or if there are issues in the logging logic.
3. src/ThumbnailApi.ts:179
  • Draft comment:
    Consider adding tests to verify that the logger outputs the expected error messages. This ensures that logging is functioning correctly and that important information is not lost.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces structured logging using Winston, replacing console logs. This is a good practice for better log management and analysis. However, the test cases for logging are not comprehensive. The sendError method in ThumbnailApi is tested, but the logger's output is not verified. This could lead to missing log entries if the logger configuration changes or if there are issues in the logging logic.
4. src/app.ts:85
  • Draft comment:
    Consider adding tests to verify that the logger outputs the expected error messages. This ensures that logging is functioning correctly and that important information is not lost.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces structured logging using Winston, replacing console logs. This is a good practice for better log management and analysis. However, the test cases for logging are not comprehensive. The sendError method in ThumbnailApi is tested, but the logger's output is not verified. This could lead to missing log entries if the logger configuration changes or if there are issues in the logging logic.

Workflow ID: wflow_7iz5bjw9b1mwMA2a


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 9512583 in 12 seconds

More details
  • Looked at 688 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. src/ExpressSetup.ts:38
  • Draft comment:
    Using process.exit can lead to abrupt shutdowns, which might not allow pending operations to complete. Consider using a more graceful shutdown approach, ensuring all pending operations are completed before exiting.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces structured logging using winston and morgan, replacing console statements. The changes seem well-implemented, but there's a minor issue with the use of process.exit in the handleExit method. Using process.exit can lead to abrupt shutdowns, which might not allow pending operations to complete. It's better to gracefully shut down the server and then exit the process.

Workflow ID: wflow_IicYUxd71YJMvUn2


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@mdellabitta mdellabitta merged commit 51a8ae3 into main Jan 5, 2025
5 checks passed
@mdellabitta mdellabitta deleted the structured-logging branch January 5, 2025 22:14
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.

1 participant