fix(logging): fix log flushing on exit MONGOSH-2883 #2547
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes log flushing on exit by making the logging flush operations asynchronous. The changes ensure that log files are properly written before the application terminates, preventing potential data loss.
- Changed
flush()anddetachLogger()methods to returnPromise<void>instead ofvoid - Added
awaitcalls for proper asynchronous handling of log flushing operations - Introduced error handling in the
start()method to ensure cleanup occurs even when exceptions are thrown
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/logging/src/types.ts | Updated interface to make flush and detachLogger methods async |
| packages/logging/src/logging-and-telemetry.ts | Implemented async flush behavior with proper log.flush() awaiting |
| packages/e2e-tests/test/e2e.spec.ts | Added tests to verify log flushing works on normal exit and exception scenarios |
| packages/cli-repl/src/cli-repl.ts | Updated calls to use async flush methods and added error handling wrapper |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const log = await readReplLogFile( | ||
| shell.output.replace(/Error: uh oh/g, '').trim() | ||
| ); |
There was a problem hiding this comment.
The error message filtering using regex replacement is fragile and could inadvertently remove legitimate log content. Consider a more robust approach to extract the log path from shell output.
| const log = await readReplLogFile( | |
| shell.output.replace(/Error: uh oh/g, '').trim() | |
| ); | |
| // Extract the log path from the shell output by finding the line that looks like a log file path | |
| const logPathLine = shell.output | |
| .split('\n') | |
| .map(line => line.trim()) | |
| .find(line => line.length > 0 && /\.log$/.test(line)); | |
| expect(logPathLine, 'Could not find log path in shell output').to.be.a('string'); | |
| const log = await readReplLogFile(logPathLine); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I mean, I had similar thoughts here as well, although I was ultimately feeling like this is just fine for a test
Depends on mongodb-js/devtools-shared#578