- 
                Notifications
    
You must be signed in to change notification settings  - Fork 35
 
chore: add additional logging around resource and memory useage #465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
4617974
              e5ae295
              1e56a3f
              f201d1a
              d7c650b
              f3bcbca
              5ebc004
              854ca01
              8974e8b
              9b440dc
              1eed376
              d6fb26c
              2c09de6
              3c3f452
              44106ee
              9659dd8
              86de75d
              5fbc7a7
              b839ad5
              c8e45da
              90172e6
              0d04fba
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -5,6 +5,7 @@ import * as vscode from "vscode" | |
| import { WebSocket } from "ws" | ||
| import { errToStr } from "./api-helper" | ||
| import { type Storage } from "./storage" | ||
| import { getMemoryLogger } from "./memoryLogger" | ||
| 
     | 
||
| // These are the template IDs of our notifications. | ||
| // Maybe in the future we should avoid hardcoding | ||
| 
        
          
        
         | 
    @@ -16,9 +17,16 @@ export class Inbox implements vscode.Disposable { | |
| readonly #storage: Storage | ||
| #disposed = false | ||
| #socket: WebSocket | ||
| #messageCount = 0 | ||
| #workspaceId: string | ||
| 
     | 
||
| constructor(workspace: Workspace, httpAgent: ProxyAgent, restClient: Api, storage: Storage) { | ||
| const logger = getMemoryLogger() | ||
| 
         
      Comment on lines
    
      25
     to 
      +26
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be better to refactor this class to take a single init object. That way, we can dependency-inject the logger, and also make it easier to add new parameters down the line. Moving the logger directly to the constructor parameters would give us a five-argument function, which feels a bit unwieldy  | 
||
| this.#storage = storage | ||
| this.#workspaceId = workspace.id | ||
| 
     | 
||
| logger.trackResourceCreated("InboxWebSocket", workspace.id) | ||
| logger.info(`Creating inbox for workspace: ${workspace.owner_name}/${workspace.name} (${workspace.id})`) | ||
| 
     | 
||
| const baseUrlRaw = restClient.getAxiosInstance().defaults.baseURL | ||
| if (!baseUrlRaw) { | ||
| 
        
          
        
         | 
    @@ -37,6 +45,8 @@ export class Inbox implements vscode.Disposable { | |
| const socketProto = baseUrl.protocol === "https:" ? "wss:" : "ws:" | ||
| const socketUrl = `${socketProto}//${baseUrl.host}/api/v2/notifications/inbox/watch?format=plaintext&templates=${watchTemplatesParam}&targets=${watchTargetsParam}` | ||
| 
     | 
||
| logger.debug(`Connecting to inbox WebSocket at: ${socketUrl}`) | ||
| 
     | 
||
| const coderSessionTokenHeader = "Coder-Session-Token" | ||
| this.#socket = new WebSocket(new URL(socketUrl), { | ||
| followRedirects: true, | ||
| 
        
          
        
         | 
    @@ -49,35 +59,72 @@ export class Inbox implements vscode.Disposable { | |
| }) | ||
| 
     | 
||
| this.#socket.on("open", () => { | ||
| logger.info(`Inbox WebSocket connection opened for workspace: ${workspace.id}`) | ||
| this.#storage.writeToCoderOutputChannel("Listening to Coder Inbox") | ||
| }) | ||
| 
     | 
||
| this.#socket.on("error", (error) => { | ||
| logger.error(`Inbox WebSocket error for workspace: ${workspace.id}`, error) | ||
| this.notifyError(error) | ||
| this.dispose() | ||
| }) | ||
| 
     | 
||
| this.#socket.on("close", (code, reason) => { | ||
| logger.info(`Inbox WebSocket closed for workspace: ${workspace.id}, code: ${code}, reason: ${reason || "none"}`) | ||
| if (!this.#disposed) { | ||
| this.dispose() | ||
| } | ||
| }) | ||
| 
     | 
||
| this.#socket.on("message", (data) => { | ||
| this.#messageCount++ | ||
| 
     | 
||
| // Log periodic message stats | ||
| if (this.#messageCount % 10 === 0) { | ||
| logger.info(`Inbox received ${this.#messageCount} messages for workspace: ${workspace.id}`) | ||
| logger.logMemoryUsage("INBOX_WEBSOCKET") | ||
| } | ||
| 
         
      Comment on lines
    
      +84
     to 
      +88
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want a little more granularity here? The current setup means that we have to wait for the first 10 messages before we log anything. I don't know how quickly the messages are coming in, but I'm worried we'll never log anything. Wondering if we could have something more logarithmic, where maybe we could have: 
  | 
||
| 
     | 
||
| try { | ||
| const inboxMessage = JSON.parse(data.toString()) as GetInboxNotificationResponse | ||
| 
     | 
||
| logger.debug(`Inbox notification received: ${inboxMessage.notification.title}`) | ||
| vscode.window.showInformationMessage(inboxMessage.notification.title) | ||
| } catch (error) { | ||
| logger.error(`Error processing inbox message for workspace: ${workspace.id}`, error) | ||
| this.notifyError(error) | ||
| } | ||
| }) | ||
| 
     | 
||
| // Log memory stats periodically | ||
| const memoryInterval = setInterval( | ||
| () => { | ||
| if (!this.#disposed) { | ||
| logger.logMemoryUsage("INBOX_PERIODIC") | ||
| } else { | ||
| clearInterval(memoryInterval) | ||
| } | ||
| }, | ||
| 5 * 60 * 1000, | ||
| ) // Every 5 minutes | ||
| } | ||
| 
     | 
||
| dispose() { | ||
                
      
                  brettkolodny marked this conversation as resolved.
               
          
            Show resolved
            Hide resolved
         | 
||
| const logger = getMemoryLogger() | ||
| 
     | 
||
| if (!this.#disposed) { | ||
| logger.info(`Disposing inbox for workspace: ${this.#workspaceId} after ${this.#messageCount} messages`) | ||
| this.#storage.writeToCoderOutputChannel("No longer listening to Coder Inbox") | ||
| this.#socket.close() | ||
| this.#disposed = true | ||
| logger.trackResourceDisposed("InboxWebSocket", this.#workspaceId) | ||
| } | ||
| } | ||
| 
     | 
||
| private notifyError(error: unknown) { | ||
| const logger = getMemoryLogger() | ||
| const message = errToStr(error, "Got empty error while monitoring Coder Inbox") | ||
| 
     | 
||
| logger.error(`Inbox error for workspace: ${this.#workspaceId}`, error) | ||
| this.#storage.writeToCoderOutputChannel(message) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,217 @@ | ||||||||||||||||||||||||||||
| import * as vscode from "vscode" | ||||||||||||||||||||||||||||
| import * as os from "os" | ||||||||||||||||||||||||||||
| import * as path from "path" | ||||||||||||||||||||||||||||
| import * as fs from "fs/promises" | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * A class for tracking memory usage and logging resource lifecycles | ||||||||||||||||||||||||||||
| * to help identify memory leaks in the extension. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| export class MemoryLogger { | ||||||||||||||||||||||||||||
| private outputChannel: vscode.OutputChannel | ||||||||||||||||||||||||||||
| private logFile: string | undefined | ||||||||||||||||||||||||||||
| private resourceCounts = new Map<string, number>() | ||||||||||||||||||||||||||||
| private startTime: number = Date.now() | ||||||||||||||||||||||||||||
| private logInterval: NodeJS.Timeout | undefined | ||||||||||||||||||||||||||||
| private disposed: boolean = false | ||||||||||||||||||||||||||||
                
      
                  brettkolodny marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
         | 
||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| constructor() { | ||||||||||||||||||||||||||||
| this.outputChannel = vscode.window.createOutputChannel("Coder Memory Logging") | ||||||||||||||||||||||||||||
| this.outputChannel.show() | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| // Setup periodic logging of memory usage | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feel like this comment isn't doing much  | 
||||||||||||||||||||||||||||
| this.startPeriodicLogging() | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Start logging memory usage periodically | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| private startPeriodicLogging(intervalMs = 60000) { | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd rather have this be extracted out into a constant so that it's easier to find and tweak down the line. I don't think it needs to exported, though  | 
||||||||||||||||||||||||||||
| if (this.logInterval) { | ||||||||||||||||||||||||||||
| clearInterval(this.logInterval) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| this.logInterval = setInterval(() => { | ||||||||||||||||||||||||||||
| if (this.disposed) return | ||||||||||||||||||||||||||||
| this.logMemoryUsage("PERIODIC") | ||||||||||||||||||||||||||||
| this.logResourceCounts() | ||||||||||||||||||||||||||||
| }, intervalMs) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Initialize the log file for persistent logging | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public async initLogFile(globalStoragePath: string): Promise<void> { | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current setup makes me worried, because we have a singleton for all the logging, but anybody can call  Wondering if this could be put in the constructor to enforce that it's initialization logic (whether that's moving the logic to the constructor directly, or making this a private method that the constructor calls) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, wait, this is an async method. We could still do some trickery to make sure things are initialized together, but the other thing that sticks out to me is that we have this init logic in a try/catch. If this method fails, the catch will suppress the error, and the  Do we want this method to return a boolean to indicate setup success?  | 
||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const logDir = path.join(globalStoragePath, "logs") | ||||||||||||||||||||||||||||
| await fs.mkdir(logDir, { recursive: true }) | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| this.logFile = path.join(logDir, `memory-log-${new Date().toISOString().replace(/[:.]/g, "-")}.txt`) | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| await this.writeToLogFile("Memory logging initialized") | ||||||||||||||||||||||||||||
| this.info("Memory logging initialized to file: " + this.logFile) | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| // Log initial memory state | ||||||||||||||||||||||||||||
| this.logMemoryUsage("INIT") | ||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||
| this.error(`Failed to initialize log file: ${err}`) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Log a new resource creation | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public trackResourceCreated(resourceType: string, id: string = ""): void { | ||||||||||||||||||||||||||||
| const count = (this.resourceCounts.get(resourceType) || 0) + 1 | ||||||||||||||||||||||||||||
| this.resourceCounts.set(resourceType, count) | ||||||||||||||||||||||||||||
| this.info(`RESOURCE_CREATED: ${resourceType}${id ? ":" + id : ""} (Total: ${count})`) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Log a resource disposal | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public trackResourceDisposed(resourceType: string, id: string = ""): void { | ||||||||||||||||||||||||||||
| const count = Math.max(0, (this.resourceCounts.get(resourceType) || 1) - 1) | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't the   | 
||||||||||||||||||||||||||||
| if (count === 0) { | ||||||||||||||||||||||||||||
| this.resourceCounts.delete(resourceType) | ||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||
| this.resourceCounts.set(resourceType, count) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| this.info(`RESOURCE_DISPOSED: ${resourceType}${id ? ":" + id : ""} (Remaining: ${count})`) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Log error with memory usage | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public error(message: string, error?: unknown): void { | ||||||||||||||||||||||||||||
| const errorMsg = error ? `: ${error instanceof Error ? error.stack || error.message : String(error)}` : "" | ||||||||||||||||||||||||||||
| const fullMessage = `[ERROR] ${message}${errorMsg}` | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| this.outputChannel.appendLine(fullMessage) | ||||||||||||||||||||||||||||
| this.writeToLogFile(fullMessage) | ||||||||||||||||||||||||||||
| this.logMemoryUsage("ERROR") | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Log info with timestamp | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public info(message: string): void { | ||||||||||||||||||||||||||||
| const fullMessage = `[INFO] ${message}` | ||||||||||||||||||||||||||||
| this.outputChannel.appendLine(fullMessage) | ||||||||||||||||||||||||||||
| this.writeToLogFile(fullMessage) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Log debug info (only to file) | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public debug(message: string): void { | ||||||||||||||||||||||||||||
| const fullMessage = `[DEBUG] ${message}` | ||||||||||||||||||||||||||||
| this.writeToLogFile(fullMessage) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Log current memory usage | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public logMemoryUsage(context: string): void { | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const memoryUsage = process.memoryUsage() | ||||||||||||||||||||||||||||
| const nodeMemoryInfo = { | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to add type annotations to these objects?  | 
||||||||||||||||||||||||||||
| rss: `${(memoryUsage.rss / 1024 / 1024).toFixed(2)}MB`, | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think, just to make this a little more readable, this could be turned into something like a  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, actually, does   | 
||||||||||||||||||||||||||||
| heapTotal: `${(memoryUsage.heapTotal / 1024 / 1024).toFixed(2)}MB`, | ||||||||||||||||||||||||||||
| heapUsed: `${(memoryUsage.heapUsed / 1024 / 1024).toFixed(2)}MB`, | ||||||||||||||||||||||||||||
| external: `${(memoryUsage.external / 1024 / 1024).toFixed(2)}MB`, | ||||||||||||||||||||||||||||
| uptime: formatDuration(process.uptime() * 1000), | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a comment explaining that we're multiplying this by 1000 because   | 
||||||||||||||||||||||||||||
| totalUptime: formatDuration(Date.now() - this.startTime), | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| const systemMemoryInfo = { | ||||||||||||||||||||||||||||
| totalMem: `${(os.totalmem() / 1024 / 1024 / 1024).toFixed(2)}GB`, | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment for the formatting – feel like this could be a   | 
||||||||||||||||||||||||||||
| freeMem: `${(os.freemem() / 1024 / 1024 / 1024).toFixed(2)}GB`, | ||||||||||||||||||||||||||||
| loadAvg: os | ||||||||||||||||||||||||||||
| .loadavg() | ||||||||||||||||||||||||||||
                
      
                  brettkolodny marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
         | 
||||||||||||||||||||||||||||
| .map((load) => load.toFixed(2)) | ||||||||||||||||||||||||||||
| .join(", "), | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| const memoryLog = `[MEMORY:${context}] Node: ${JSON.stringify(nodeMemoryInfo)} | System: ${JSON.stringify(systemMemoryInfo)}` | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How important is it that the direct JSON output be human-readable out of the box, as opposed to throwing the entire output onto a single line?  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I follow since It's already pretty readable to me. Are you suggesting pretty printing the output, or changing the structure of the output completely to not be JSON?  | 
||||||||||||||||||||||||||||
| this.outputChannel.appendLine(memoryLog) | ||||||||||||||||||||||||||||
| this.writeToLogFile(memoryLog) | ||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||
| this.outputChannel.appendLine(`[ERROR] Failed to log memory usage: ${err}`) | ||||||||||||||||||||||||||||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you stringify the error directly, you'll only get the error type and the top-level error message. Do we want to output the   | 
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Log the current counts of active resources | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| private logResourceCounts(): void { | ||||||||||||||||||||||||||||
| const counts = Array.from(this.resourceCounts.entries()) | ||||||||||||||||||||||||||||
| .map(([type, count]) => `${type}=${count}`) | ||||||||||||||||||||||||||||
| 
         
      Comment on lines
    
      +147
     to 
      +148
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this can be cleaned up a little bit.   | 
||||||||||||||||||||||||||||
| .join(", ") | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| const message = `[RESOURCES] Active resources: ${counts || "none"}` | ||||||||||||||||||||||||||||
| this.outputChannel.appendLine(message) | ||||||||||||||||||||||||||||
| this.writeToLogFile(message) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Write to log file | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| private async writeToLogFile(message: string): Promise<void> { | ||||||||||||||||||||||||||||
| if (!this.logFile) return | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const timestamp = new Date().toISOString() | ||||||||||||||||||||||||||||
| await fs.appendFile(this.logFile, `${timestamp} ${message}\n`) | ||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||
| // Don't recursively call this.error to avoid potential loops | ||||||||||||||||||||||||||||
| this.outputChannel.appendLine(`[ERROR] Failed to write to log file: ${err}`) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Show the log in the output channel | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public show(): void { | ||||||||||||||||||||||||||||
| this.outputChannel.show() | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Dispose of the logger | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public dispose(): void { | ||||||||||||||||||||||||||||
| this.disposed = true | ||||||||||||||||||||||||||||
| if (this.logInterval) { | ||||||||||||||||||||||||||||
| clearInterval(this.logInterval) | ||||||||||||||||||||||||||||
| this.logInterval = undefined | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
         
      Comment on lines
    
      +185
     to 
      +188
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this if statement isn't necessary.   | 
||||||||||||||||||||||||||||
| this.logMemoryUsage("DISPOSE") | ||||||||||||||||||||||||||||
| this.outputChannel.dispose() | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Format duration in milliseconds to a human-readable string | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| function formatDuration(ms: number): string { | ||||||||||||||||||||||||||||
| const seconds = Math.floor((ms / 1000) % 60) | ||||||||||||||||||||||||||||
| const minutes = Math.floor((ms / (1000 * 60)) % 60) | ||||||||||||||||||||||||||||
| const hours = Math.floor((ms / (1000 * 60 * 60)) % 24) | ||||||||||||||||||||||||||||
| const days = Math.floor(ms / (1000 * 60 * 60 * 24)) | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| return `${days}d ${hours}h ${minutes}m ${seconds}s` | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| // Singleton instance | ||||||||||||||||||||||||||||
| let instance: MemoryLogger | undefined | ||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Get or initialize the memory logger instance | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| export function getMemoryLogger(): MemoryLogger { | ||||||||||||||||||||||||||||
| if (!instance) { | ||||||||||||||||||||||||||||
| instance = new MemoryLogger() | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| return instance | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| 
         
      Comment on lines
    
      +206
     to 
      +217
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if I'm missing something here, but do we need all this? This feels very Java-y to me 
        Suggested change
       
    
 If there's always going to be a singleton instance, I feel like we could export that instance directly, so that the rest of the file doesn't have to deal with  Depending on what kinds of dependency injection we want to do, we could have a   | 
||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this comment isn't doing much, especially if we end up needing to add more cleanup logic that isn't logging-related down the line