chore: add additional logging around resource and memory useage #465
Conversation
| } | ||
|
|
||
| logger.info(`Found SSH socks port: ${port}, searching for process`) | ||
| const processes = await find("port", port) |
There was a problem hiding this comment.
In my testing I could not get this function call to return any processes using the given port. But running lsof -i :<port> showed that it was indeed in use. Not sure if this is relevant but figured it was worth mentioning
There was a problem hiding this comment.
Tried to do the best job I could, but I think there might be a few VSCode-specific nuances I'm missing
I can circle around for a second review + QA later, but I don't know if @code-asher has the bandwidth to give a good rubber stamp
| // Singleton instance | ||
| let instance: MemoryLogger | undefined | ||
|
|
||
| /** | ||
| * Get or initialize the memory logger instance | ||
| */ | ||
| export function getMemoryLogger(): MemoryLogger { | ||
| if (!instance) { | ||
| instance = new MemoryLogger() | ||
| } | ||
| return instance | ||
| } |
There was a problem hiding this comment.
I'm not sure if I'm missing something here, but do we need all this? This feels very Java-y to me
| // 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?
| /** | ||
| * Start logging memory usage periodically | ||
| */ | ||
| private startPeriodicLogging(intervalMs = 60000) { |
There was a problem hiding this comment.
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
| /** | ||
| * Initialize the log file for persistent logging | ||
| */ | ||
| public async initLogFile(globalStoragePath: string): Promise<void> { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
| @@ -56,10 +57,21 @@ export class WorkspaceProvider implements vscode.TreeDataProvider<vscode.TreeIte | |||
| // Calling this while already refreshing or not visible is a no-op and will | |||
| // return immediately. | |||
| async fetchAndRefresh() { | |||
| if (this.fetching || !this.visible) { | |||
| const logger = getMemoryLogger() | |||
There was a problem hiding this comment.
Similar comment about injecting the logger at the class level, rather than doing it for each method
| this._onDidChangeTreeData.fire(item) | ||
| } | ||
|
|
||
| dispose(): void { |
There was a problem hiding this comment.
Do we want to make sure this turns into a no-op if this is called multiple times?
| const eventSource = new EventSource(metadataUrl.toString(), { | ||
| fetch: createStreamingFetchAdapter(restClient.getAxiosInstance()), | ||
| }) | ||
|
|
||
| let disposed = false | ||
| let eventCount = 0 |
There was a problem hiding this comment.
Nit: could this be moved down to be right above the data listener?
| eventCount++ | ||
|
|
||
| // Log periodic updates | ||
| if (eventCount % 50 === 0) { |
There was a problem hiding this comment.
How did we go about deciding the thresholds for when to log different things? I'm seeing a lot of different values, which I could see making sense, but I'm not 100% sure
There was a problem hiding this comment.
The thresholds were decided somewhat arbitrarily based on how often it'd be logging to try and strike a balance of useful to spammy
|
I have not reviewed the code yet, but I am happy to take a look. I will hold off until the second round, unless y'all are OK with me potentially making the same comments hahaha Tangentially, wondering if we have had folks try profiling (unfortunately it seems VS Code only has CPU profiling, although the process explorer does show current memory usage). https://github.com/Microsoft/vscode/wiki/Performance-Issues I do have some concerns that this is an unusual way to profile a process and may not get us the data we need, but I have not had a chance to see if there is a way to toggle standard Node profiling on the extension host, so maybe this is as good as it gets for our circumstances. Also, do we have any rotation on the new log file to prevent it from growing unbounded? And do we need the separate file? Could we just log everything to the output channel? Possibly using |
Adds a
MemoryLoggerclass to the extension to log the memory and resource usage of the extension during various parts of the extension's life.Closes #429
Warning
Various aspects of the code in this PR were written with the help of AI.
I personally attest to the quality, but I am not as familiar with this code base so some extra scrutinizing would be appreciated.