-
Notifications
You must be signed in to change notification settings - Fork 14
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!: docker log to stdout and minor logging tweaks #951
Conversation
WalkthroughThe changes involve formatting adjustments in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DockerEntryPoint
participant State
User->>DockerEntryPoint: Start container
DockerEntryPoint->>DockerEntryPoint: Initialize configuration
DockerEntryPoint->>State: Call OnStart
State->>State: Execute catchup replay
State-->>State: Log warning if error occurs
State-->>DockerEntryPoint: Complete process
DockerEntryPoint-->>User: Container running
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@@ -52,14 +51,14 @@ if [ ! -d "$TMHOME/config" ]; then | |||
fi | |||
|
|||
# Start tenderdash in the background | |||
tenderdash "$@" & | |||
tenderdash "$@" 2>&1 & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you want to send all stderr from Tenderdash? We should probably let Tenderdash control output and adjust the logger to use stdout instead of stderr. There might be some confusion in the future when you do some stderr output from Tenderdash and do not see it when you run a docker container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My AI junior says:
For server software running on Linux, it is generally recommended to send logs to `stderr`. This is because `stdout` is typically used for regular output, while `stderr` is used for logging errors and diagnostic information. Sending logs to `stderr` helps to separate log messages from the normal output of the program, making it easier to manage and analyze logs.
Here's a summary of the best practices:
- **stdout**: Use for regular program output, such as the results of a command or data that the program is designed to produce.
- **stderr**: Use for logging errors, warnings, and diagnostic information.
By following this convention, you can ensure that logs are properly handled and can be redirected or processed separately from the main output of your server software.
So, Tenderdash behavior to send logs to stderr is correct. For a particular use case of Docker, I decided to join these streams for usability.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Issue being fixed or feature implemented
We must redirect stderr to stdout before grepping output of
docker logs
command.It's quite hard to use.
What was done?
How Has This Been Tested?
Locally
Breaking Changes
logs captured from
docker logs
must now be read from stdout instead of stderr.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Chores