Skip to content
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

chore: add additional logging around resource and memory useage #465

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
4617974
feat: create memory logger
brettkolodny Apr 1, 2025
e5ae295
fix: format
brettkolodny Apr 1, 2025
1e56a3f
feat: log information regarding network updates
brettkolodny Apr 1, 2025
f201d1a
chore: improve logging for findSSHProcessID
brettkolodny Apr 1, 2025
d7c650b
chore: add memory logging to WorkspacesProvider
brettkolodny Apr 1, 2025
f3bcbca
chore: add logging to inbox
brettkolodny Apr 1, 2025
5ebc004
chore: improve logging around extension starting and stopping
brettkolodny Apr 1, 2025
854ca01
chore: add logging around workspace fetching and refetching logic
brettkolodny Apr 1, 2025
8974e8b
fix: resolve lint warnings
brettkolodny Apr 1, 2025
9b440dc
chore: remove non-useful avgload information
brettkolodny Apr 2, 2025
1eed376
chore: remove unneeded type annotations
brettkolodny Apr 4, 2025
d6fb26c
fix: add radix to number parse
brettkolodny Apr 4, 2025
2c09de6
chore: rename loop attempts variable
brettkolodny Apr 4, 2025
3c3f452
chore: remove unneeded try-catch
brettkolodny Apr 4, 2025
44106ee
chore: remove unneeded comment
brettkolodny Apr 4, 2025
9659dd8
fix: remove unneeded try catch
brettkolodny Apr 4, 2025
86de75d
fix: move interval to class level and clear on dispose
brettkolodny Apr 4, 2025
5fbc7a7
chore: remove unneeded comment
brettkolodny Apr 4, 2025
b839ad5
Merge branch 'main' into brett-429/add-debuggin-to-the-plugin
brettkolodny Apr 4, 2025
c8e45da
fix: fix lint warnings
brettkolodny Apr 4, 2025
90172e6
chore: update version
brettkolodny Apr 4, 2025
0d04fba
fix: properly set version
brettkolodny Apr 4, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,27 @@ import { makeCoderSdk, needToken } from "./api"
import { errToStr } from "./api-helper"
import { Commands } from "./commands"
import { CertificateError, getErrorDetail } from "./error"
import { getMemoryLogger } from "./memoryLogger"
import { Remote } from "./remote"
import { Storage } from "./storage"
import { toSafeHost } from "./util"
import { WorkspaceQuery, WorkspaceProvider } from "./workspacesProvider"

export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
// Initialize the memory logger right when the extension starts.
const logger = getMemoryLogger()
await logger.initLogFile(ctx.globalStorageUri.fsPath)

// Log initial memory usage and activation
logger.info("CODER extension activating")
logger.logMemoryUsage("EXTENSION_ACTIVATE")

// Register disposal of the logger when the extension deactivates
ctx.subscriptions.push({ dispose: () => logger.dispose() })

// Log extension mode
logger.info(`Extension mode: ${extensionModeToString(ctx.extensionMode)}`)

// The Remote SSH extension's proposed APIs are used to override the SSH host
// name in VS Code itself. It's visually unappealing having a lengthy name!
//
Expand All @@ -25,9 +40,13 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
vscode.extensions.getExtension("codeium.windsurf-remote-openssh") ||
vscode.extensions.getExtension("ms-vscode-remote.remote-ssh")
if (!remoteSSHExtension) {
logger.error("Remote SSH extension not found, cannot activate Coder extension")
vscode.window.showErrorMessage("Remote SSH extension not found, cannot activate Coder extension")
throw new Error("Remote SSH extension not found")
}

logger.info(`Found Remote SSH extension: ${remoteSSHExtension.id}`)

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const vscodeProposed: typeof vscode = (module as any)._load(
"vscode",
Expand Down Expand Up @@ -225,4 +244,26 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
}
}
}

logger.info("Coder extension activation complete")
}

function extensionModeToString(mode: vscode.ExtensionMode): string {
switch (mode) {
case vscode.ExtensionMode.Development:
return "Development"
case vscode.ExtensionMode.Production:
return "Production"
case vscode.ExtensionMode.Test:
return "Test"
default:
return `Unknown (${mode})`
}
}

// Add deactivation handler to log memory usage on extension shutdown
Copy link
Collaborator

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

export function deactivate(): void {
const logger = getMemoryLogger()
logger.info("Coder extension deactivating")
logger.logMemoryUsage("EXTENSION_DEACTIVATE")
}
49 changes: 48 additions & 1 deletion src/inbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ProxyAgent } from "proxy-agent"
import * as vscode from "vscode"
import { WebSocket } from "ws"
import { errToStr } from "./api-helper"
import { getMemoryLogger } from "./memoryLogger"
import { type Storage } from "./storage"

// These are the template IDs of our notifications.
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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) {
Expand All @@ -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,
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

  • First message
  • First five messages
  • Every 10 messages after that


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() {
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)
}
}
221 changes: 221 additions & 0 deletions src/memoryLogger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
import * as fs from "fs/promises"
import * as os from "os"
import * as path from "path"
import * as vscode from "vscode"

/**
* 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

constructor() {
this.outputChannel = vscode.window.createOutputChannel("Coder Memory Logging")
this.outputChannel.show()

// Setup periodic logging of memory usage
Copy link
Collaborator

Choose a reason for hiding this comment

The 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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> {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 initLogFile to change where the log output will go. If we need to output to multiple files, couldn't this risk crossing the streams a bit, especially since we would've already kicked off startPeriodicLogging at this point?

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 activate function will still be allowed to move on as if though everything is fine

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the || 1 ensure that the Math.max won't ever be necessary?

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 = {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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`,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 bytesToMegabytes helper

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually, does prettyBytes take care of this for us already? Figured we might as well use it if it's already a dependency

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),
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 formatDuration expects milliseconds, and process.uptime gives back seconds?

totalUptime: formatDuration(Date.now() - this.startTime),
}

const systemMemoryInfo = {
totalMem: `${(os.totalmem() / 1024 / 1024 / 1024).toFixed(2)}GB`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment for the formatting – feel like this could be a bytesToGigabytes helper

freeMem: `${(os.freemem() / 1024 / 1024 / 1024).toFixed(2)}GB`,
loadAvg: os
.loadavg()
.map((load) => load.toFixed(2))
.join(", "),
}

const memoryLog = `[MEMORY:${context}] Node: ${JSON.stringify(nodeMemoryInfo)} | System: ${JSON.stringify(systemMemoryInfo)}`
Copy link
Collaborator

Choose a reason for hiding this comment

The 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? JSON.stringify supports formatting the output to make it more readable

Copy link
Author

Choose a reason for hiding this comment

The 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}`)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 err.stack property, too, if it exists?

}
}

/**
* 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this can be cleaned up a little bit. Array.from already has support for mapping built in via its second parameter, and will also auto-convert the map into an entries iterable

.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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this if statement isn't necessary. clearInterval has support for no-ops if it's passed an undefined value

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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
// Singleton instance
let instance: MemoryLogger | undefined
/**
* Get or initialize the memory logger instance
*/
export function getMemoryLogger(): MemoryLogger {
if (!instance) {
instance = new MemoryLogger()
}
return instance
}
export const memoryLogger = new MemoryLogger();

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 undefined cases

Depending on what kinds of dependency injection we want to do, we could have a getDefaultMemoryLogger function that returns the instance?

Loading