Logging adapter and usage #545
Conversation
Implement a centralized logging system to improve debugging capabilities and separate logging concerns from the Storage class. - Add logger module with configurable debug/info levels - Support runtime configuration via coder.verbose setting - Provide OutputChannelAdapter for VS Code integration - Add ArrayAdapter for isolated unit testing - Include performance benchmarks in tests - Maintain backward compatibility via Storage delegation - Update all components to use centralized logger The logger responds to configuration changes without requiring extension restart and includes source location tracking for debug messages. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…een without config
| try { | ||
| this.outputChannel.appendLine(message); | ||
| } catch { | ||
| // Silently ignore - channel may be disposed |
There was a problem hiding this comment.
We could perhaps console.log as a last-ditch effort
| try { | ||
| this.outputChannel.clear(); | ||
| } catch { | ||
| // Silently ignore - channel may be disposed |
|
|
||
| private setupConfigListener(): void { | ||
| // In test environment, vscode.workspace might not be available | ||
| if (!vscode.workspace?.onDidChangeConfiguration) { |
There was a problem hiding this comment.
Would it be possible to always mock these? I feel like that would be preferable so we can minimize the number of test-specific branches in the production code, which increases the chance we miss something in tests.
Also, thinking out loud, this is only a problem because of our custom test setup right? When you scaffold an extension by default, even in unit tests vscode is available I think? Or am I wrong about that lol
|
|
||
| private updateLogLevel(): void { | ||
| // In test environment, vscode.workspace might not be available | ||
| if (!vscode.workspace?.getConfiguration) { |
| return; | ||
| } | ||
| const config = vscode.workspace.getConfiguration("coder"); | ||
| const verbose = config.get<boolean>("verbose", false); |
There was a problem hiding this comment.
Do you think it would make sense to add a setting for levels instead? I could see one day adding a level setting, then we would have both levels and verbose.
Which is itself not that strange, I think plenty of tools have both a verbose and log level flag, but it would match the remote extensions which have dropdowns for level settings.
At a minimum I think we would want trace, debug, info, warn, and error (even if we only use info right now, just so we have no problems with the numbering in the future where a setting of 2 used to mean none but would now mean info).
Also, we need to add the setting to the package.json as well right?
| } | ||
| } | ||
|
|
||
| export class NoOpAdapter implements LogAdapter { |
There was a problem hiding this comment.
Is logger performance really something we need to worry about? If so, could we create a separate benchmarking suite, maybe with mitata or something? I think to settle on hard performance thresholds in a unit test we would need to see the performance data first, although maybe we would want a separate pipeline for enforcing performance constraints.
But also, the current test is comparing the difference between doing nothing and pushing to an array, which I am not sure tells us anything we could act on, and neither adapter is used in production anyway.
| setAdapter(adapter: LogAdapter): void { | ||
| if (process.env.NODE_ENV !== "test") { | ||
| throw new Error("setAdapter can only be called in test environment"); | ||
| } | ||
| if (this.adapter !== null) { | ||
| throw new Error( | ||
| "Adapter already set. Use reset() first or withAdapter() for temporary changes", | ||
| ); | ||
| } | ||
| this.adapter = adapter; | ||
| } | ||
|
|
||
| withAdapter<T>(adapter: LogAdapter, fn: () => T): T { | ||
| const previous = this.adapter; | ||
| this.adapter = adapter; | ||
| try { | ||
| return fn(); | ||
| } finally { | ||
| this.adapter = previous; | ||
| } | ||
| } | ||
|
|
||
| reset(): void { | ||
| if (process.env.NODE_ENV !== "test") { | ||
| throw new Error("reset can only be called in test environment"); | ||
| } | ||
| this.adapter = null; | ||
| this.level = LogLevel.INFO; | ||
| if (this.configListener) { | ||
| this.configListener.dispose(); | ||
| this.configListener = null; | ||
| } | ||
| // Re-setup config listener for next test | ||
| this.updateLogLevel(); | ||
| this.setupConfigListener(); | ||
| } |
There was a problem hiding this comment.
If this is all purely for tests, could we refactor in a way that does not add unused API to the production code? Maybe by extending the logger class or something?
I also see that the only places we use withAdapter is to test withAdapter, so it exists only for itself and seems like we could just delete it entirely.
For reset, we could create new instances instead, might be better to do that anyway to ensure unit tests are isolated.
| initialize(outputChannel: vscode.OutputChannel): void { | ||
| if (this.adapter !== null) { | ||
| throw new Error("Logger already initialized"); | ||
| } | ||
| this.adapter = new OutputChannelAdapter(outputChannel); | ||
| } |
There was a problem hiding this comment.
It feels a little backwards to me that the logger is built around a generic adapter, but also has knowledge about a specific adapter, if that makes sense? Could we do something like new Logger(new OutputChannelAdapter(output)) or something like that (or allow setting the adapter outside of tests).
|
|
||
| // Export types for testing | ||
| export type Logger = typeof logger; |
There was a problem hiding this comment.
If we only need it for testing, we should move it to the test file!
| /** | ||
| * Compatibility method for Logger interface used by headers.ts and error.ts. | ||
| * Delegates to the logger module. | ||
| */ | ||
| public writeToCoderOutputChannel(message: string): void { | ||
| logger.info(message); |
There was a problem hiding this comment.
Oh wait I thought you changed headers.ts and error.ts. Can we delete this function now?
No description provided.