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

feat(jetbrains): show workspace class in backend control center #12568

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

andreafalzetti
Copy link
Contributor

@andreafalzetti andreafalzetti commented Aug 31, 2022

Description

This PR adds support in JetBrains Backend Control Center for displaying the Gitpod's Workspace Class, to be accessible and easily discoverable.

The PR also includes small changes to the layout of the Performance Tab to make it easier for the user to understand what metrics are related to the Workspace (relevant) and what metrics are related to the Node (less relevant for them).

Current After this PR After this PR (Missing Class) After this PR (hover on shared node resources)
Screenshot 2022-09-01 at 23 11 04 Screenshot 2022-09-02 at 22 58 12 Screenshot 2022-09-02 at 23 03 59 Screenshot 2022-09-02 at 22 58 08

Considerations

  • I also tried adding the class on its own row, below "Workspace", but it would look odd

Related Issue(s)

Fixes #12160

How to test

  1. Start a workspace with IntelliJ: https://andreafalz6b6c136ea3.preview.gitpod-dev.com/
  2. Check the backend control center, you should see the workspace class as in the screenshots above

Release Notes

JetBrains: Provide workspace class info in Backend Control Center 

Documentation

Werft options:

  • /werft with-preview

@akosyakov
Copy link
Member

akosyakov commented Sep 2, 2022

I also tried adding the class on its own row, below "Workspace", but it would look odd

@andreafalzetti What if we add at very top?

Also we don't need to prefix with Class: instead if it is missing don't show a row, if it is present then show ${name}: ${description}

@akosyakov
Copy link
Member

akosyakov commented Sep 2, 2022

👍 I like the separation of Workspace/Node.

  • We are missing disk resource for Workspace, disk resource for Node is misleading.
  • Load Average is related to overall Node, not to workspace. We should move it down too?

@andreafalzetti
Copy link
Contributor Author

andreafalzetti commented Sep 2, 2022

@akosyakov I also prefer the separation Workspace/Node and the class if currently displayed with the Workspace header. If the class displayName is not available, the header is Workspace otherwise it will be displayed as Workspace (Name: Description). See screenshot.

We are missing disk resource for Workspace, disk resource for Node is misleading.

I can look into this as a separate PR 👍

What if we add at very top?

Currently, it's not possible to add elements at the very top, because pingAndLaControl has order="first"

Also we don't need to prefix with Class:

There is not Class: prefix (that was a test screenshot but I didn't go for that option)

@andreafalzetti andreafalzetti force-pushed the andreafalzetti/integrate-jetbrains-12160 branch from 5dfcd95 to 07e06ea Compare September 2, 2022 11:02
@roboquat roboquat added size/L and removed size/M labels Sep 2, 2022
@akosyakov
Copy link
Member

Currently, it's not possible to add elements at the very top, because pingAndLaControl has order="first"

Can we add it after pingAndLaControl before Workspace section?

@andreafalzetti
Copy link
Contributor Author

andreafalzetti commented Sep 2, 2022

Can we add it after pingAndLaControl before Workspace section?

@akosyakov yes, let's try! I think it's a matter of preference so I've tried 3 layouts and took a screenshot so we can vote/discuss.

A B C
Screenshot 2022-09-02 at 15 24 19 Screenshot 2022-09-02 at 15 25 34 Screenshot 2022-09-02 at 15 26 42

My thoughts:

A: It feels that we now have 2 sections (workspace/node) and the class is outside of the workspace section, so I am not inclined towards this layout very much. At the same time pretty much all data is around the workspace, so I am not against it.

B: The class is just below "workspace" re-enforcing that that value is related. Also, other controls have a label (CPU, Latency, etc). This is the first component without a label in this UI so the fact that is below workspace makes me think it's more obvious that we're referring to the class..

C: This is a bit more explicit that those specs are for the workspace. I've tested with very long text it will just make the widget width larger but it's still visible.

I prefer B and C, but I would go with B because it looks cleaner the UI with 2 lines instead of a long one. What about you @akosyakov?

@akosyakov
Copy link
Member

I prefer B and C, but I would go with B because it looks cleaner the UI with 2 lines instead of a long one. What about you @akosyakov?

Let's go with B.

@felladrin
Copy link
Contributor

felladrin commented Sep 2, 2022

I think it's a matter of preference so I've tried 3 layouts and took a screenshot so we can vote/discuss.

I liked more the option B. The separation seems clearer.
If you could add an icon before the words "Workspace" and "Node", just to make the separation even more clear, it would be great. (Alternatively, I think that adding : behind those works would already help.) (Or making them bold.)

@akosyakov
Copy link
Member

@andreafalzetti I wonder maybe below Node we can add some note, like that are resources shared among all users of the node or something like that. cc @loujaybee

@felladrin
Copy link
Contributor

I wonder maybe below Node we can add some note, like that are resources shared among all users of the node or something like that.

How about "Shared resources in this machine:" in place of the word "Node"?

@andreafalzetti
Copy link
Contributor Author

@andreafalzetti I wonder maybe below Node we can add some note, like that are resources shared among all users of the node or something like that. cc @loujaybee

If we want to keep the UI clean and intuitive, we could consider:

  1. Replacing "Node" for something more specific like @felladrin suggested
  2. Use a tooltip on "Node" header to provide extra info and not have the text always visible. It won't be discovered by most users, but it will not add extra text always visible.

@akosyakov
Copy link
Member

Shared resources in this machine:

What do you think about it as a user? I don't argue that node is cleaner. But maybe something like Resources available to all workspaces running on the node: or Resources shared between workspaces running on the node.

I am not sure about using machine, since we use this word in other context as well. Like in VS Code Browser each origin is a machine.

@felladrin
Copy link
Contributor

Resources shared between workspaces running on the node sounds good!

@andreafalzetti andreafalzetti force-pushed the andreafalzetti/integrate-jetbrains-12160 branch from 07e06ea to e5c0cc7 Compare September 2, 2022 22:07
@andreafalzetti
Copy link
Contributor Author

andreafalzetti commented Sep 2, 2022

@akosyakov @felladrin thanks both for the feedback 🙏

I've updated the PR with my proposed solution, I went with something sort but still that gives up the idea. I also went for the tooltip because adding more text would make the UI more crowded and once the info is read, is no longer needed (we can re-enforce the concept with external docs, etc):

I've updated the screenshot variations in the main comment, please have a look at let me know if you would like to change anything 👂

@andreafalzetti andreafalzetti force-pushed the andreafalzetti/integrate-jetbrains-12160 branch from e5c0cc7 to 581ff68 Compare September 2, 2022 22:31
Copy link
Contributor

@felladrin felladrin left a comment

Choose a reason for hiding this comment

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

Code checked and tested! ✅

image

I'll put it on hold to give the chance to @akosyakov to complete his review.

/hold

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

lgtm

@andreafalzetti
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit c75d066 into main Sep 5, 2022
@roboquat roboquat deleted the andreafalzetti/integrate-jetbrains-12160 branch September 5, 2022 09:29
@roboquat roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: IDE IDE change is running in production deployed Change is completely running in production editor: jetbrains release-note size/L team: IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate JetBrains backend control center with workspace classes
4 participants