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

Webapp Preview: Cluster Overview with sparklines #23600

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

koszti
Copy link
Member

@koszti koszti commented Sep 29, 2024

Description

This PR adds the cluster statistics page into the preview UI dashboard, maintaining identical functionality to the existing UI while incorporating the latest Material UI components. Partially addressing #22697

Technical details

The dashboard fetches cluster metrics every second from the /ui/api/stats endpoint and visualizes them using the mui-x/charts SparkLineChart component.

Note that there is some overlap with PR #23230, which implements authentication. Currently, this feature supports only fixed authentication types; however, it will be compatible with all authentication methods once the aforementioned PR is merged.

UPDATE: The PR also updates all dependencies to latest. Including to upgrade eslint to 9.x that requires to replace .eslintrc.cjs to eslint.config.js.

Screenshots

The displayed metrics are generated values and do not accurately represent real-world scenarios.

Screenshot 2024-09-30 012731

Screenshot 2024-09-30 012803

@cla-bot cla-bot bot added the cla-signed label Sep 29, 2024
@github-actions github-actions bot added the ui Web UI label Sep 29, 2024
@koszti koszti force-pushed the webapp-preview-dashboard branch 3 times, most recently from 2510d59 to 970f58c Compare September 30, 2024 11:35
@koszti koszti force-pushed the webapp-preview-dashboard branch from 970f58c to 4197fa4 Compare October 11, 2024 08:41
@koszti koszti requested a review from wendigo October 15, 2024 22:04
* limitations under the License.
*/
export interface ApiResponse {
ok: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this field if we have a status?

Copy link
Member Author

Choose a reason for hiding this comment

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

dropped

}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
async get(url: string, params: Record<string, any> = {}): Promise<ApiResponse>
Copy link
Contributor

Choose a reason for hiding this comment

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

can we instead use some high level library that does all of these things?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will make it to use axios very soon.

Copy link
Member Author

@koszti koszti Nov 17, 2024

Choose a reason for hiding this comment

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

it's now using axios with generic get/post wrappers. Dealing with HTTP errors and JSON serialisation are done by axios automatically.

@koszti koszti force-pushed the webapp-preview-dashboard branch 5 times, most recently from b4f7c70 to a879d97 Compare November 17, 2024 18:41
@koszti koszti force-pushed the webapp-preview-dashboard branch from a879d97 to 5437bc7 Compare November 17, 2024 19:28
Copy link

github-actions bot commented Dec 9, 2024

This pull request has gone a while without any activity. Tagging for triage help: @mosabua

@github-actions github-actions bot added the stale label Dec 9, 2024
@wendigo
Copy link
Contributor

wendigo commented Dec 12, 2024

I'm looking into it right now

@wendigo
Copy link
Contributor

wendigo commented Dec 12, 2024

Overall LGTM. There is an issue with rendering metrics cards: some have values on the left and some in the middle:

Screenshot 2024-12-12 at 11 02 18

@wendigo
Copy link
Contributor

wendigo commented Dec 12, 2024

I think that we can merge it and improve as follow up since this is a preview UI.

@wendigo
Copy link
Contributor

wendigo commented Dec 12, 2024

@mosabua wdyt?

@mosabua
Copy link
Member

mosabua commented Dec 12, 2024

Agreed .. lets merge and continue in further PRs.

@mosabua
Copy link
Member

mosabua commented Dec 12, 2024

I cant merge myself at the moment since I am busy with summit and cant fix things if anything goes wrong. Go ahead @wendigo if you are available.. otherwise I will do later today.

@wendigo wendigo merged commit da79aed into trinodb:master Dec 12, 2024
92 checks passed
@github-actions github-actions bot added this to the 468 milestone Dec 12, 2024
@mosabua
Copy link
Member

mosabua commented Dec 12, 2024

Added to release notes...

@mosabua
Copy link
Member

mosabua commented Dec 12, 2024

Also @koszti please note https://trino.io/development/process#pull-request-and-commit-guidelines- for next time ..

In this case it could have been

  • Add cluster overview to the Preview Web UI.

@koszti koszti deleted the webapp-preview-dashboard branch December 16, 2024 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants