✨ Unified logging in extension #595
Conversation
|
""" WalkthroughThis change replaces all direct console logging across the codebase with structured logging using the Winston library. Logger instances are injected into major classes and utilities, and all logging calls now use appropriate log levels. Configuration and type definitions are updated to support the new logging system, and related dependencies are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Extension as VSCode Extension
participant Logger as Winston Logger
participant Analyzer as AnalyzerClient
participant Solution as SolutionServerClient
participant Commands as Commands Module
Extension->>Logger: createLogger(paths)
Extension->>Analyzer: new AnalyzerClient(..., logger)
Extension->>Solution: new SolutionServerClient(..., logger)
Extension->>Commands: registerAllCommands(state, logger)
Analyzer->>Logger: logger.info/debug/error(...)
Solution->>Logger: logger.info/debug/error(...)
Commands->>Logger: logger.info/debug/error(...)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Estimated code review effort3 (~45 minutes) Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
vscode/src/extension.ts (1)
344-344: Consider migrating remaining console.error to logger.This is the last remaining console.error call in the extension. Consider whether this should also use the logger for consistency, or if console.error is intentionally kept for critical activation failures.
Since the logger may not be available during activation failures, this console.error might be intentional. However, you could consider creating a basic logger earlier in the activation process if you want full consistency.
vscode/src/client/analyzerClient.ts (1)
152-152: Consider migrating RPC trace to Winston logger.The RPC connection trace is still using
consoledirectly. Consider using the Winston logger for consistency.-this.analyzerRpcConnection.trace(rpc.Trace.Messages, console, false); +this.analyzerRpcConnection.trace(rpc.Trace.Messages, { + log: (message: string) => this.logger.debug(`RPC trace: ${message}`) +}, false);vscode/src/utilities/logger-migration-plan.md (1)
127-127: Minor grammar improvement for compound adjective.The static analysis tool correctly identified a compound adjective that should be hyphenated.
-- Phase 1: High priority error handling (Week 1) +- Phase 1: High-priority error handling (Week 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
agentic/src/clients/solutionServerClient.ts(25 hunks)agentic/src/tools/filesystem.ts(4 hunks)agentic/src/workflows/interactiveWorkflow.ts(7 hunks)agentic/tests/tools.test.ts(1 hunks)shared/src/types/types.ts(1 hunks)vscode/package.json(2 hunks)vscode/src/client/analyzerClient.ts(20 hunks)vscode/src/client/types.ts(0 hunks)vscode/src/commands.ts(19 hunks)vscode/src/extension.ts(10 hunks)vscode/src/extensionState.ts(2 hunks)vscode/src/utilities/configuration.ts(1 hunks)vscode/src/utilities/logger-migration-plan.md(1 hunks)vscode/src/utilities/logger.ts(1 hunks)
💤 Files with no reviewable changes (1)
- vscode/src/client/types.ts
🧰 Additional context used
🧠 Learnings (2)
vscode/src/client/analyzerClient.ts (1)
Learnt from: ibolton336
PR: konveyor/editor-extensions#558
File: vscode/src/webviewMessageHandler.ts:228-229
Timestamp: 2025-06-30T15:26:08.027Z
Learning: Provider configuration status should be determined by actual provider settings (API keys, credentials, etc.) and should not be tied to active analysis profiles. Provider configuration is global and independent of which analysis profile is currently active.
vscode/package.json (2)
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: test/package.json:11-122
Timestamp: 2025-07-16T12:49:03.553Z
Learning: Security vulnerabilities flagged in package.json reviews may be related to the main package.json file rather than test-specific package.json files in the konveyor/editor-extensions project.
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: test/package.json:11-122
Timestamp: 2025-07-16T12:49:03.553Z
Learning: The electron package is required for the test environment in the konveyor/editor-extensions project, specifically for the test/package.json file.
🧬 Code Graph Analysis (3)
agentic/tests/tools.test.ts (2)
agentic/src/tools/filesystem.ts (1)
FileSystemTools(21-147)agentic/src/fsCache.ts (1)
SimpleInMemoryCache(5-40)
agentic/src/workflows/interactiveWorkflow.ts (3)
vscode/src/client/tracer.ts (1)
logger(60-65)agentic/src/types.ts (1)
PendingUserInteraction(87-90)agentic/src/tools/filesystem.ts (1)
FileSystemTools(21-147)
agentic/src/tools/filesystem.ts (2)
agentic/src/types.ts (1)
KaiFsCache(109-118)vscode/src/client/tracer.ts (1)
logger(60-65)
🪛 LanguageTool
vscode/src/utilities/logger-migration-plan.md
[uncategorized] ~127-~127: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...works properly ## Timeline - Phase 1: High priority error handling (Week 1) - Phase 2: Medi...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
- GitHub Check: Build (linux)
🔇 Additional comments (56)
shared/src/types/types.ts (1)
267-267: LGTM! Clear constant for output channel naming.The addition of
KONVEYOR_OUTPUT_CHANNEL_NAMEprovides a consistent identifier for the VS Code output channel used by the Winston logging system.agentic/tests/tools.test.ts (1)
5-17: LGTM! Proper test logger configuration.The Winston logger setup correctly matches the updated
FileSystemToolsconstructor signature. The configuration with debug level, timestamp, and JSON formatting is appropriate for testing purposes.vscode/src/extensionState.ts (2)
11-11: LGTM! Winston import for logging integration.The Winston import supports the logger property addition to the ExtensionState interface.
28-28: LGTM! Centralized logger integration.Adding the logger property to
ExtensionStateenables consistent structured logging across the extension components.vscode/src/utilities/configuration.ts (1)
37-37: LGTM! Aligned with Winston log levels.The change to return a plain string and use lowercase "debug" correctly aligns with Winston's standard npm log levels.
agentic/src/tools/filesystem.ts (5)
8-8: LGTM! Winston logger import.The Winston Logger import supports the structured logging migration.
22-22: LGTM! Logger property addition.The private logger property enables structured logging within the FileSystemTools class.
27-36: LGTM! Proper logger initialization.The logger parameter addition to the constructor and child logger creation with component scoping follows Winston best practices.
109-109: LGTM! Improved error logging.The change from
console.errortothis.logger.errorprovides structured logging with both the error message and error object for better debugging.
141-141: LGTM! Consistent error logging.The structured logging approach is consistently applied to the writeFileTool error handling.
vscode/src/utilities/logger.ts (5)
1-8: LGTM! Well-structured imports and dependencies.The imports are clean and properly organized, bringing in necessary Winston components and VS Code integrations.
10-16: Good logger factory function design.The function properly accepts extension paths and creates appropriate log file path for the logger.
17-36: Excellent Winston logger configuration.The logger setup includes:
- Dynamic log level from configuration
- JSON format with timestamps and error stacks
- File transport with proper rotation (10MB, 3 files)
- VS Code output channel integration
This provides comprehensive logging capabilities for the extension.
38-45: Appropriate development console transport.The conditional console transport for development mode with colorized output is a good practice for debugging.
47-49: Good initialization logging.The initial log message confirms logger creation, which is helpful for debugging logger setup issues.
vscode/package.json (2)
460-475: Appropriate log level configuration update.The migration from custom log levels to Winston's standard npm log levels is correct and follows industry best practices. The default level change from "DEBUG" to "debug" maintains equivalent functionality.
694-697: Dependency Versions Verified – No Security Advisories Found
- File vscode/package.json (lines 694–697):
[email protected]is the latest release.[email protected]is the only published version.- An isolated
npm auditrun against these exact versions produced no advisories.All listed Winston dependencies are current and have no known moderate-or-higher vulnerabilities.
agentic/src/clients/solutionServerClient.ts (5)
4-4: Correct Winston Logger import.The Logger import is properly added to support the structured logging migration.
41-49: Excellent logger integration.The logger is properly injected via constructor and a child logger is created with the component name for better log organization. This follows best practices for structured logging.
53-55: Appropriate log level for disabled service.Using
infolevel for the disabled service message is appropriate as it's informational rather than an error.
74-90: Good error handling and logging.The connection success and error logging is well-structured with appropriate log levels. Error details are properly captured and logged.
100-112: Proper disconnect logging.The disconnect process is appropriately logged with info level for normal operations and error level for exceptions.
agentic/src/workflows/interactiveWorkflow.ts (5)
29-29: Correct Winston Logger import.The Logger import supports the structured logging migration in the workflow.
84-95: Excellent logger dependency injection.The constructor properly accepts and initializes the logger with a child logger for component identification. This follows the established pattern for structured logging.
103-103: Proper logger propagation to tools.The logger is correctly passed to FileSystemTools, ensuring consistent logging throughout the workflow components.
308-308: Appropriate error logging.The error logging for user interaction failures uses the correct log level and provides useful context.
373-424: Excellent debug logging in router edge.The debug logging provides comprehensive state information for troubleshooting workflow routing decisions. The structured logging approach makes this information much more useful than console output.
vscode/src/extension.ts (5)
27-27: Correct logger utility import.The createLogger import supports the centralized logging implementation.
78-95: Excellent logger integration in extension state.The logger is properly created and injected into both AnalyzerClient and SolutionServerClient constructors, and stored in the extension state for broader access. This centralizes logging configuration and ensures consistent behavior across components.
131-131: Appropriate async error logging.The solution server connection error is properly logged using the structured logger instead of console output.
157-186: Good configuration change logging.The configuration change handlers use appropriate log levels (info) for tracking configuration modifications and their effects.
241-241: Proper critical error logging.Command registration errors are logged with the error level, which is appropriate for critical initialization failures.
vscode/src/client/analyzerClient.ts (13)
24-24: Logger import added for structured logging migration.Good addition of the Winston Logger type for the structured logging migration.
46-53: Logger injection and child logger creation implemented correctly.The constructor properly accepts the logger parameter and creates a child logger with the "AnalyzerClient" component label, which follows structured logging best practices.
58-58: Server state logging enhanced with structured information.The logging statement properly includes context about the state transition, which is valuable for debugging server lifecycle issues.
106-106: Server lifecycle logging consistently migrated to Winston.All server startup and lifecycle events are now properly logged with appropriate log levels (info for normal operations, error for failures).
Also applies to: 112-112, 116-116, 120-120, 124-124
128-128: RPC connection logging properly structured.Socket connection events and RPC message handling are now logged with appropriate detail and log levels.
Also applies to: 131-131, 134-134, 140-140, 143-143, 146-146, 149-149
154-154: RPC notification and request logging properly implemented.All RPC events are now logged with appropriate detail and structured format.
Also applies to: 157-157, 159-159, 163-163, 166-166, 169-169
174-175: Workspace command execution logging enhanced.Command execution logging now includes structured information about the command and its arguments, which is valuable for debugging.
Also applies to: 184-184, 187-187
202-202: Health check logging properly structured.Health check operations are now logged with appropriate detail and error handling.
Also applies to: 210-212, 223-223
235-235: Server process and analysis logging migrated correctly.Server process lifecycle and analysis operations are now logged with Winston instead of console methods.
Also applies to: 268-270, 291-291
313-313: Server shutdown logging properly implemented.Server shutdown process is now logged with appropriate detail for debugging.
Also applies to: 318-318, 332-332
341-341: Analysis execution logging comprehensive.Analysis workflow logging includes proper context and error handling with structured information.
Also applies to: 363-363, 381-381, 387-387
399-403: Analysis result processing and error handling well-structured.Analysis result processing and error scenarios are logged with appropriate detail and context.
Also applies to: 406-406, 425-425, 454-454, 483-483
544-544: Error logging for missing analyzer binary.Error logging for missing analyzer binary is properly structured with context.
vscode/src/utilities/logger-migration-plan.md (1)
1-130: Comprehensive logging migration plan with clear strategy.This is an excellent migration plan that provides:
- Clear migration strategy with specific replacement patterns
- Prioritized file list with progress tracking
- Implementation guidance for different scenarios
- Timeline with phased approach
- Testing strategy
The plan demonstrates thorough planning for the Winston logging migration.
vscode/src/commands.ts (10)
69-69: Logger type import added for structured logging.Proper addition of Winston Logger type for the migration.
73-78: Function signature updated to accept logger parameter.The commandsMap function signature correctly updated to accept the logger parameter, enabling structured logging throughout all command handlers.
85-85: Server management command logging properly migrated.All server start/stop/restart commands now use structured logging with appropriate error context.
Also applies to: 96-96, 104-104, 119-119, 130-130
135-135: Analysis and solution command logging enhanced.Analysis and solution generation commands now include structured logging with relevant context and metadata.
Also applies to: 144-144, 150-150, 176-176, 183-183
320-322: Agent workflow error handling with structured logging.Error handling in the agent workflow properly uses structured logging with error context and stack traces.
Also applies to: 341-341, 358-358, 396-396
414-414: Success rate and file change notifications properly logged.Success rate calculations and file change notifications now use structured logging with appropriate context.
Also applies to: 418-418, 433-433, 437-437, 442-442, 446-446, 451-451
488-488: Document handling and analysis details logging migrated.Document opening and analysis detail operations now use structured logging.
Also applies to: 601-601
667-668: Command registration with child logger implementation.Proper creation of child logger for commands component and structured error handling in command registration.
Also applies to: 674-674, 677-677, 688-688
719-719: File processing utility function updated with logger.The processModifiedFile function signature correctly updated to accept logger parameter and uses structured logging for warnings.
Also applies to: 775-775
304-304: Logger parameter properly passed to utility function.The logger is correctly passed to the processModifiedFile function, maintaining consistency in the logging chain.
djzager
force-pushed
the
unified-logging
branch
2 times, most recently
from
e897640 to
c8a5e30
Compare
- Everything (that actually uses the new logger) is output to the vscode output channel AND a konveyor-extension.log - Logging level is determined by configuration Signed-off-by: David Zager <[email protected]>
|
Looking good locally for me. |
With this change we can, throughout the extension codebase, use the logger to write to both the vscode output console AND a file in konveyor-logs in such a way that everything from commands, interactive workflows, interactions with the solution server, and even analyzer queries are logged as they happen.
Resolves #542
Summary by CodeRabbit
New Features
Refactor
Chores