Migrate logging from custom Logger to LogTape#554
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the extension's logging infrastructure from a custom Logger implementation to LogTape, a modern structured logging library. The migration simplifies the codebase by removing custom logging code and adopting a standard logging solution.
Changes:
- Replaced custom
Loggerclass with LogTape's structured logging API - Updated all logging call sites to use LogTape's template literal syntax and structured properties
- Removed logger parameters from constructors across providers, linters, and managers in favor of local logger initialization
Reviewed changes
Copilot reviewed 38 out of 40 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/logging.ts | Implements LogTape initialization and provides extension logger factory |
| src/logtape-vscode-sink.ts | Creates custom LogTape sink for VS Code OutputChannel integration |
| src/test/logTestUtils.ts | Adds testing utilities for capturing and inspecting LogTape output |
| src/test/logtape.test.ts | Adds comprehensive tests for LogTape integration |
| src/logger.ts | Removes deprecated custom Logger implementation |
| src/test/logger.test.ts | Removes tests for deprecated custom Logger |
| src/extension.ts | Updates to use LogTape and removes logger parameter passing |
| src/linter/*.ts | Updates all linters to use LogTape template literal syntax |
| src/providers/*.ts | Updates all providers to initialize loggers locally |
| src/test/*.test.ts | Removes logger injection from test instantiations |
| package.json | Adds @logtape/logtape dependency and moves test-cli to devDependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (err instanceof Error && err.stack) { | ||
| this.logger.error(err.stack); | ||
| } | ||
| this.logger.error`Exception caught: ${err}`; |
There was a problem hiding this comment.
Error logging loses stack trace information. The previous implementation logged both the error message and stack trace separately, which is helpful for debugging. Consider including stack trace in the properties object when err is an Error instance.
| this.logger.error`Exception caught: ${err}`; | |
| if (err instanceof Error) { | |
| this.logger.error`Exception caught while executing ctags: ${err.message} ${{ error: err, stack: err.stack, name: err.name }}`; | |
| } | |
| else { | |
| this.logger.error`Exception caught while executing ctags: ${String(err)}`; | |
| } |
| if (!extension) { | ||
| throw new Error(`Extension ${extensionID} not found`); | ||
| const errorMessage = `Extension ${extensionID} not found`; | ||
| this.logger.fatal("Extension not found", { extensionId: extensionID }); |
There was a problem hiding this comment.
Fatal log is issued after the error message variable is created but the variable is never used in the thrown Error. The logging happens but the actual Error thrown uses a redundant string literal instead of errorMessage variable.
| this.logger.fatal("Extension not found", { extensionId: extensionID }); | |
| this.logger.fatal(errorMessage, { extensionId: extensionID }); |
- Update @logtape/logtape from ^1.3.6 to ^2.0.0 - Replace deprecated 'level' property with 'lowestLevel' in logger config (breaking change in LogTape 1.0.0)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
https://logtape.org/