-
Notifications
You must be signed in to change notification settings - Fork 28
Add agent metadata statusbar to monitor resource usage #555
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
base: main
Are you sure you want to change the base?
Conversation
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.
Awesome! Left some minor thoughts. We should add a changelog entry as well, and it might make sense to mention that if they want to change which shows in the toolbar, they can reorder the metadata in the Terraform since we show the first one.
* The status bar item updates dynamically based on changes to the agent's metadata, | ||
* and hides itself if no metadata is available or an error occurs. | ||
*/ | ||
private createAgentMetadataStatusBar( |
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.
Thinking out loud, not sure if this is a good move or not, but would it make sense to make this part of WorkspaceMonitor
? Such that new WorkspaceMonitor()
would monitor both the workspace and the agent metadata and update the status bar with both pieces of info.
But on the other hand, I could also see an argument that monitoring the workspace and monitoring the agent are really two different things and should not be colocated.
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 had thought of that but indeed I thought this is not really a Workspace
item, it's purely an Agent
item
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.
yeah fair enough
src/remote.ts
Outdated
this.storage.writeToCoderOutputChannel( | ||
formatMetadataError(agentWatcher.error), | ||
); | ||
statusBarItem.hide(); |
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.
Would it make sense to show the error here? Or just a warning icon or something with the error in the tooltip maybe?
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.
Hmmm tbh I am not entirely sure how frequent an error like this can happen.
If we show an error, it could be annoying if it keeps popping up frequently. Perhaps adding an error icon with some label and the tooltip can include the error message is good (while still logging of course). Though I don't love having a "useless" status bar item since the status bar is already crowded 🤔
Which is why I favor the current approach, what do you think?
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.
My thinking is that if we think it is important enough to show metadata in the status bar, it is important enough to show errors getting that metadata.
Definitely agree there should be no popups or anything like that, that would be super annoying.
Thinking about it from the original request, the user wanted to be able to see disk space in the status bar. Suppose one day there was an error and we could not fetch the metadata, likely the user wants to be aware of that (I am projecting of course lol)
I prefer the no logo, or if we can make the logo smaller, such as 70% of its original size. |
@rachelmullenax can we have a square logo for use cases like this? |
I wonder if some generic icon indicating a remote resource would work better than the logo? Like a cloud icon or something. |
#191
Looks like the following:
The statusbar creation happens in
remote.ts#setup
which AFAIK seems to be called only once per session/extension activation. The creation of this statusbar happens at the very end when everything goes smoothly.