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.

Sign up for GitHub

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

Jump to bottom

Pagination on Usage page #12501

Merged
merged 1 commit into from
Aug 31, 2022
Merged

Pagination on Usage page #12501

merged 1 commit into from
Aug 31, 2022

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Aug 30, 2022 •
edited
Loading

Description

Adding server-side pagination to list usage.

❗ There is a test commit to set the perPage attribute to a value of 3, which makes this PR handy to test. That should be removed before merging.

Screen Shot 2022-08-30 at 17 43 20

Notes

  • sort by Timestamp is removed as agreed-on
  • drive-by fix of [ 1 ] page links to make them clickable
  • total credits used is computed on DB instead on frontend

Related Issue(s)

Fixes #12388

How to test

Release Notes

Speed up the Usage page for teams.

Documentation

Werft options:

  • /werft with-preview
  • /werft with-payment

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-alex-slowness-loading-usage-list-12388.1 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-alex-slowness-loading-usage-list-12388.2 because the annotations in the pull request description changed
(with .werft/ from main)

@AlexTugarev AlexTugarev linked an issue Aug 30, 2022 that may be closed by this pull request
Slowness loading usage list view #12388
Closed
@roboquat roboquat added size/XXL and removed size/S labels Aug 30, 2022
@AlexTugarev AlexTugarev force-pushed the alex/slowness-loading-usage-list-12388 branch from 0f1c12b to be92c4f Compare August 30, 2022 15:40
@AlexTugarev
Copy link
Member Author

AlexTugarev commented Aug 30, 2022 •
edited
Loading

/hold
because of test commit 7ca14c8

  • removed

@AlexTugarev AlexTugarev force-pushed the alex/slowness-loading-usage-list-12388 branch from be92c4f to f87e6b6 Compare August 30, 2022 15:55
@AlexTugarev
Copy link
Member Author

/hold cancel

@AlexTugarev AlexTugarev marked this pull request as ready for review August 30, 2022 15:56
@AlexTugarev AlexTugarev requested a review from a team August 30, 2022 15:56
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Aug 30, 2022
@svenefftinge
Copy link
Member

Hey Alex! I can look into this. Do you have a team set up (invite link?) that has a bit of usage shown already?

easyCZ
easyCZ reviewed Aug 31, 2022
View reviewed changes
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.

Similar to your previous PR, this PR would greatly benefit from being split into multiple. The following steps lean themselves very nicely.

  1. Change proto
  2. Implement the UsageService logic, defaulting to Page Size of "Everything" to maintain current behaviour when the number of results is not specified
  3. Change Server/dashboard to specify number of results per page, and paginate

As it stands, the big PR causes any changes to the underlying API protocol to force you to change everything else. It's better to agree and commit to the API spec and only then progress with the changes.

The current version (all in one) also suffers from undefined behaviour as the changes rollout across dashboard/server/usage. We do not know if the UI will handle only one page of changes properly if it doesn't yet know about the request per page logic.

components/usage-api/usage/v1/usage.proto Outdated Show resolved Hide resolved
components/usage-api/usage/v1/usage.proto Show resolved Hide resolved
@AlexTugarev
Copy link
Member Author

The current version (all in one) also suffers from undefined behaviour as the changes rollout across dashboard/server/usage.

@easyCZ, thanks for the comment. I think we know about the impact pretty well. Considering the timeline, I'd prefer to solve this as a vertical.

It's better to agree and commit to the API spec and only then progress with the changes.

In theory this is great, I'm kind of skeptical this works out nicely in a three layered cake. Once you meet the needy requirements from implementing the dashboard, you'll likely start out with the next iteration.

Let's move general discussions out of PRs, please!

svenefftinge
svenefftinge reviewed Aug 31, 2022
View reviewed changes
Copy link
Member

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

Looks super good 🧡 Just a few smaller things.

components/dashboard/src/Pagination/Pagination.tsx
};
const prevPage = () => {
if (currentPage !== 1) setCurrentPage(currentPage - 1);
if (currentPage !== 1) setPage(currentPage - 1);
Copy link
Member

Choose a reason for hiding this comment

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

not directly related to this PR #12537

components/dashboard/src/teams/TeamUsage.tsx Outdated Show resolved Hide resolved
components/dashboard/src/teams/TeamUsage.tsx Show resolved Hide resolved
components/gitpod-protocol/src/usage.ts Outdated Show resolved Hide resolved
components/server/ee/src/workspace/gitpod-server-impl.ts Show resolved Hide resolved
components/usage/pkg/db/workspace_instance_usage.go Outdated Show resolved Hide resolved
components/usage/pkg/db/workspace_instance_usage_test.go Show resolved Hide resolved
components/usage/pkg/apiv1/usage_test.go Show resolved Hide resolved
@AlexTugarev
Copy link
Member Author

AlexTugarev commented Aug 31, 2022 •
edited
Loading

/hold
because of test commit

  • removed

@AlexTugarev
Copy link
Member Author

@svenefftinge the navigation between months is fixed now.

jldec
jldec approved these changes Aug 31, 2022
View reviewed changes
Copy link
Contributor

@jldec jldec left a comment

Choose a reason for hiding this comment

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

LGTM - thank you for this @AlexTugarev
/hold because of test commit

@AlexTugarev AlexTugarev force-pushed the alex/slowness-loading-usage-list-12388 branch from b9c025a to 0960113 Compare August 31, 2022 13:02
@AlexTugarev
Copy link
Member Author

/hold cancel

@roboquat roboquat merged commit a8d0889 into main Aug 31, 2022
@roboquat roboquat deleted the alex/slowness-loading-usage-list-12388 branch August 31, 2022 13:14
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Aug 31, 2022
@gtsiolis gtsiolis mentioned this pull request Sep 1, 2022
Closed
@jldec jldec mentioned this pull request Sep 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svenefftinge svenefftinge svenefftinge left review comments

@easyCZ easyCZ easyCZ left review comments

@jldec jldec jldec approved these changes

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/XXL team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slowness loading usage list view
5 participants
@AlexTugarev @svenefftinge @jldec @easyCZ @roboquat