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

log: panics during server startup don't make it to the console #91700

Closed
andreimatei opened this issue Nov 10, 2022 · 2 comments · Fixed by #91823
Closed

log: panics during server startup don't make it to the console #91700

andreimatei opened this issue Nov 10, 2022 · 2 comments · Fixed by #91823
Assignees
Labels
A-logging In and around the logging infrastructure. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-server-and-security DB Server & Security

Comments

@andreimatei
Copy link
Contributor

andreimatei commented Nov 10, 2022

Panics that happen during server startup go into a black hole. They don't make it to the console even when running with --logtostderr. This has annoyed me several times now. The panic is printed in cockroach-stderr.log, though.
To repro, put a panic at the beginning of sql.Server.PreStart(), for example. I think it has something to do with this stderr capturing business.

Jira issue: CRDB-21389

@andreimatei andreimatei added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-logging In and around the logging infrastructure. T-server-and-security DB Server & Security labels Nov 10, 2022
@knz
Copy link
Contributor

knz commented Nov 11, 2022

This is working more-or-less as intended.

The contract is this:

  1. top priority is that we can retrieve these panics in debug zip for prod clusters. Therefore, they must be written to a file.
  2. the go runtime is built such that it cannot write errors to both a file and stderr. (That "capturing business" exists only because the go runtime is being obtuse about this).
  3. therefore, we have chosen to redirect them to a file to satisfy (1) at the expense of stderr.

You can work around this with --log='capture-stray-errors: {enable: false}' for your situation, but then debug zip won't see the panic any more.

There's a couple of things we could do about this:

  • patch the go runtime (since we now can do this. It wasn't when we designed the log package)
  • add some recovers here and there to redirect the panic object through the logging stack, but that would be just playing whack-a-mole, as long as *: ban the go keyword in non-test code #58164 is open.

@andreimatei
Copy link
Contributor Author

andreimatei commented Nov 11, 2022

You can work around this with --log='capture-stray-errors: {enable: false}'

Good to know.

add some recovers here and there to redirect the panic object through the logging stack, but that would be just playing whack-a-mole, as long as #58164 is open.

I think a recover around the goroutine doing Server.PreStart() would go a long way towards making me happier :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-logging In and around the logging infrastructure. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-server-and-security DB Server & Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants