Include the contents of logs when postgres fails to start or initdb fails #84
Conversation
|
@fergusstrange do you have any idea what's up with the test failures in CI here? They appear to all work for me locally. |
|
Hey @mackinleysmith I'm not entirely sure without digging in a little more. Happy to do this if you struggle but I'll point you to a way to replicate as a first course. Running the below with docker should replicate your build issues, which only seem to happen on Alpine Linux.
|
|
Thanks for your help @fergusstrange, I think this is ready for proper review now! |
|
Did something similar here: There are two more places you probably want to log failures (starting and stopping postgres) Also, I'm not sure you want to return the full logs in the error itself (they are printed to stdout/stderr) |
|
@same-id thanks for reviewing! I gave some attention to logging failures when postgres fails to start right here: https://github.com/fergusstrange/embedded-postgres/pull/84/files#diff-fb14f8cabb78ed661f4fed8900d372aee1b1a3fa6a73e893e165d0641c430d50R191-R194... or were you thinking about something else? I could imagine some value in printing the logs if postgres fails to stop, I'll try to get that added. Finally, the reason I'm doing this is that error logs such as SHMALL overflows are not being printed to stdout/stderr. I was unable to find some other way to access this information when postgres fails to start, and the problem was compounded by our usage of a temp directory for embedded-postgres to run in which is deleted after the test finishes. We needed to do this in order to deal with parallelism of Thanks! |
|
Just gonna go ahead and bump this. I spent some time attempting to add this functionality to the stop routine for postgres, but I haven't been able to come up with a good way to test it, and I'm not sure it's actually so valuable. The problems my org has been running into are entirely related to starting Postgres. @fergusstrange what do you think? |
|
Hey @mackinleysmith this is a fairly comprehensive change and I've been a little light on time recently. Can you bear with me a little longer whilst I get the time to fully test this out? |
|
No worries! I'll check back in a month or so just to make sure this doesn't fall through the cracks. I love this package, but the error handling surrounding Thanks for the heads up! |
|
Hey @fergusstrange, any thoughts on this as we move into the new year? |
|
Hey @mackinleysmith finally getting around to this. Thank you so much for the effort. There's been some movement on master so going to close & reopen this to force a build and see if it's all good still. Will try and review the code this evening and get it merged. Cheers |
|
Alas, you'll just need to rebase to get some of the go 1.18 features now. |
prepare_database.go
Outdated
| @@ -45,7 +45,8 @@ func defaultInitDatabase(binaryExtractLocation, runtimePath, pgDataDir, username | |||
| postgresInitDBProcess.Stdout = logger | |||
|
|
|||
| if err = postgresInitDBProcess.Run(); err != nil { | |||
| return fmt.Errorf("unable to init database using '%s': %w", postgresInitDBProcess.String(), err) | |||
| logContent, _ := readLogsOrTimeout(logger) // we want to preserve the original error | |||
There was a problem hiding this comment.
We'll need to handle this error and bubble it.
There was a problem hiding this comment.
In theory I'm not sure I agree on the bubbling, since we are already returning a custom error from fmt.Errorf and I really don't want the error from postgres to be overwritten by the failure to read the logs (which very well might not exist, and isn't critical if they don't). Therefore, I've attempted to meet you in the middle by adding the log reading error to the end of the log content instead of bubbling. Does that suffice?
There was a problem hiding this comment.
@fergusstrange how you feeling on this?
There was a problem hiding this comment.
Yeap I think that's a great solution.
|
Great work on this @mackinleysmith and apologies for the tardy reviews. I've merged, will do some manual testing quickly, then will cut a release for you. |
|
Released! Nice one @mackinleysmith thanks 🙏 |
…ails (fergusstrange#84) * fix: print log contents when postgres fails to start * fix: flush the logger before attempting to read * test: failure is included in error when postgres fails to start * fix: include log contents with initialization errors * fix: flush the logger before reading * refactor: read by file name * test: logs are output on initialization failure * fix: os -> ioutil for older versions of Go * fix: os -> ioutil for older versions of Go * lint: stop cuddling 😆 * fix: bail if we can't read the logs after 10s * refactor: move log reading into function * refactor: use timeout version for startPostgres too * test: logging method * test: error case * test: logging method content * fix: imports * feat: include log reading error in log output
This builds upon #77, which turned out to not quite yield enough information in many cases. By reading the logger upon failure, we can gain insight into the cause without having to dive into temp directories (which may be cleaned up by your test runner before you have an opportunity to read them!)
Related issue: #78.