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

[usage] Use workspaceClass displayName in Usage view #14327

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

laushinka
Copy link
Contributor

@laushinka laushinka commented Nov 1, 2022

Description

This uses the displayName instead of the id

Screenshot 2022-11-01 at 16 27 04

Note: This shows `Default` because it's using the preview env config.

Related Issue(s)

Relates #13362

How to test

  1. Join this team OR create your own team with the name "Gitpod" (uppercase 'G') and run some workspaces.
  2. Check out the /usage page and see the workspace class name.

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-lau-credits-info-13362.2 because the annotations in the pull request description changed
(with .werft/ from main)

@roboquat roboquat added the size/S label Nov 1, 2022
@laushinka laushinka marked this pull request as ready for review November 1, 2022 15:31
@laushinka laushinka requested a review from a team November 1, 2022 15:31
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Nov 1, 2022
@@ -81,6 +90,10 @@ function UsageView({ attributionId }: UsageViewProps) {
return "Prebuild";
};

const getDisplayName = (workspaceClass: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

This should show the workspaceClass as a fallback when the workspace class is no longer defined. The usage records go very far back in time so it's possible a class no longer exists when we're looking at usage history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@laushinka laushinka force-pushed the lau/credits-info-13362 branch from 57ebff1 to d8b9fe1 Compare November 1, 2022 15:37
@easyCZ
Copy link
Member

easyCZ commented Nov 1, 2022

/hold for Q

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

While this fixes the issue, it does result in reliance on the supportedClasses call. This is not a problem in itself, but if someone wanted to consume the API, and get all of the data, they wouldn't now have the display name populated.

It would be nice to also extend the UsageRecords response to include the supported class there. This would involve populating it when we read out the usage records from the DB, or even populating those in the DB at the time of usage record creation.

Is it possible to maybe show the ID as a tooltip of the workspace class? Or somehow show both the WorkspaceClassID and the WorkspaceClassName? We use the ID in other places so I expect it should remain accessible somehow. CC @gtsiolis

@laushinka
Copy link
Contributor Author

laushinka commented Nov 1, 2022

Thanks for the 👍🏼 @easyCZ!

It would be nice to also extend the UsageRecords response to include the supported class there.
We use the ID in other places so I expect it should remain accessible somehow.

These are good nudges and also invites a bigger question for me: what consistency would we like to show the user? As you correctly linked the id being used in the gitpod.yml, we show the displayName instead in the account preferences for users to select. If I understand correctly, the preference view was why this issue was created, so that users can see that what they selected is correctly reflected in the usage list.
But it's good to rethink what we want to actually achieve here.
Also cc-ing @jldec in this thread.

Meanwhile I will unhold as this decision is not a blocker for the PR, and we can have follow-up issues from here.

/unhold

@roboquat roboquat merged commit 5e09a4d into main Nov 1, 2022
@roboquat roboquat deleted the lau/credits-info-13362 branch November 1, 2022 17:31
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 2, 2022
@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 2, 2022

Thanks for the ping @easyCZ! 🏓

Some thoughts:

  1. From a UX perspective, I'd avoid the tooltip if possible for accessibility reasons, and more. This could be something to consider while improving the workspaces list, see Improve workspaces page #12033. Cc @jldec
  2. I'd think that going with the ID is preferable and more scalable, but this will become clearer as the product matures and we add more workspace classes that we also need to surface in the dashboard, which could force us to move away from relying on the display name. 🌱
  3. The PR linked above (Cc @Furisto @atduarte) is still pending and there's an ongoing discussion to skip adding this configuration in .gitpod.yml altogether, see Allow specification of workspace class in project settings #10963 (comment) (Cc @svenefftinge) and relevant discussion (internal) (Cc @atduarte).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants