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

output the internal errors of zap logger to stderr #24

Merged
merged 4 commits into from
Dec 7, 2021

Conversation

zhaoxinyu
Copy link
Contributor

Solve the problem in cdc issue 3362 .

At present, when we write log messages to a log file which resides in a full disk, pingcap/log just silently discard the i/o error "no space left on device". As a result, users can not conveniently identify the root cause.

Similarly, when other internal errors happen in zap logger, we should output them to stderr as soon as possible.

@zhaoxinyu
Copy link
Contributor Author

/cc @nolouch

@nolouch
Copy link
Member

nolouch commented Nov 22, 2021

How do you capture the error when your program running in background, and terminal is closed?

@zhaoxinyu
Copy link
Contributor Author

zhaoxinyu commented Nov 22, 2021

How do you capture the error when your program running in background, and terminal is closed?

@nolouch For this question, we can redirect the stderr of a process (like cdc server) to a different file in another disk partition.

Moreover, for a daemon process which is suspected of running abnormally, users usually can do troubleshooting as follows:

  • Find the complete process command using ps aux | grep $pid.
  • Execute the complete command manually in terminal.
  • Then he/she can identify the root cause easily from the terminal output.

In brief, i think it's better to write the internal error of zap logger to stderr, and leave it to the user of pingcap/log lib to determine whether to redirect stderr to a different file or run the process manually.

log.go Outdated Show resolved Hide resolved
@zhaoxinyu zhaoxinyu requested a review from nolouch December 3, 2021 06:14
Copy link
Member

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

lgtm

config.go Outdated
// ErrorOutputPath is a path to write internal logger errors to.
// If this field is not set, the internal logger errors will be sent to the same file as in File field.
// Note: if we want to output the logger errors to stderr, we can just set this field to "stderr"
ErrorOutputPath string
Copy link
Member

@nolouch nolouch Dec 6, 2021

Choose a reason for hiding this comment

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

I think you should provide the toml and json tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nolouch Done

@zhaoxinyu zhaoxinyu requested a review from nolouch December 6, 2021 08:49
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.

2 participants