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

mgr/dashboard: multisite sync status card for rgw overview dashboard #52915

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

aaSharma14
Copy link
Contributor

@aaSharma14 aaSharma14 commented Aug 10, 2023

Signed-off-by: Aashish Sharma aasharma@redhat.com

Fixes: https://tracker.ceph.com/issues/62496

Screenshot from 2023-08-10 12-45-39

Screenshot from 2023-08-10 12-45-47

Screencast.from.2023-08-17.17-26-07.webm

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@aaSharma14 aaSharma14 requested a review from a team as a code owner August 10, 2023 07:02
@aaSharma14 aaSharma14 force-pushed the rgw-multisite-sync-card branch 2 times, most recently from 22bfff1 to 789221f Compare August 10, 2023 08:01
Copy link
Contributor

@Pegonzal Pegonzal left a comment

Choose a reason for hiding this comment

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

Thanks @aaSharma14 for the great work on this, it's a cool and great addition for sure!

After testing it for a bit, let me give my thoughts:

Looks like it takes a little bit to load, it would be nice to have a loading spinner while the sync status card finishes loading, to let the user know something is going on and it just randomly appears

screen-capture-_18_.mp4

Regarding of the layout itself, I'm still not a fan of, I think a better placement would have been to have the realm1 -> zg1-realm1 on top of both cards, just below Multisite sync status since the zones below will share a namespace.

Some other things I see:

image-mh

  1. I think the naming Primary Source Zone is not the most appropiate. Data will flow from the master zone into the rest of the zones in the zonegroup and viceversa, so not sure if this is the best naming for it.
  2. Same as before, but a part from that I think if we are naming the first as "source" I don't think the second card should be "source" as well, can be confussing as you would expect to be a "destination" or something similar
  3. This part is confusing to me and kinda out of place to the rest of the layout, I think a better indication that there is anything to sync for the metadata would be appriciated and I'm not sure of what does the {zone is master} is supposed to mean
  4. No syncing is being made yet the status shows as syncing
  5. I think the flow of the arrows can be missleading as it kinda looks like the syncing goes from the realm1 into the zone1-zg1-realm1 , also I think it pops up too much compared to the rest of the layout when is not one of the main things to be looking at
  6. If progress is at 100% I would expect that it said done, completed or anything similar, to clarify that it has actually finished. This also goes with point 4
  7. The text value inside the progress bar should be white as in the other green badges

Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

Nice work, @aaSharma14 !

Capitalization

We should stick to a single capitalization style, either "English Style or Title Case" or "Just first letter":
image

Text Color in Progress Bar

It should be lighter for readability purposes and for consistency with the badge style:
image

"Status" Badge

Usually when a text field is limited to a subset of values (enum-like), we replace that with a badge (e.g.: OSD status: in, up, out, down, etc...).

image

Reduce info displayed

Instead of showing 2 properties in the Data Sync (status and sync progress), can't we reduce that to a single property:

  • Title: Data Sync Status (or even just Data Sync)
  • Body:
    • Just badge/label showing syncing (the current status). No need to title that with status, it can be inferred.
    • Just sync progress bar. No need to title that with progress (if you see "120 km/h" on a car dashboard, you don't need to be told that's a "Speed").

BTW, is it expected that status is "syncing" and progress is "100%"? That could be counter-intuitive. To me syncing means that progress is less that 100% (unless the default status is "syncing"... in that very case "syncing" equals to "everything ok"? What would be the other sync statuses?).

Important things first

From that tooltip it looks to me that the most important thing there is the "data is caught up with source", isn't it? I'd put that first and try to summarize it as a single concept: "in sync". However that sounds contradictory with the sync status, which is "syncing". Can we replace that "syncing" with "in sync", and only "syncing" when the progress is less that "100%"?

image

Regarding the 0/128 and 128/128, I'd replace that with progress bars or percentages:

  • If 0/128 means nothing to do for "full sync", then 100% there.
  • If 128/128 means nothing to do for "incremental sync", then another 100% there.

My guess is that RGW is showing a developer status instead of summarizing that in operator-meaningful terms.

@aaSharma14
Copy link
Contributor Author

Nice work, @aaSharma14 !

Capitalization

We should stick to a single capitalization style, either "English Style or Title Case" or "Just first letter": image

Text Color in Progress Bar

It should be lighter for readability purposes and for consistency with the badge style: image

"Status" Badge

Usually when a text field is limited to a subset of values (enum-like), we replace that with a badge (e.g.: OSD status: in, up, out, down, etc...).

image

Reduce info displayed

Instead of showing 2 properties in the Data Sync (status and sync progress), can't we reduce that to a single property:

* Title: `Data Sync Status` (or even just `Data Sync`)

* Body:
  
  * Just badge/label showing `syncing` (the current status). No need to title that with `status`, it can be inferred.
  * Just sync progress bar. No need to title that with `progress` (if you see "120 km/h" on a car dashboard, you don't need to be told that's a "Speed").

BTW, is it expected that status is "syncing" and progress is "100%"? That could be counter-intuitive. To me syncing means that progress is less that 100% (unless the default status is "syncing"... in that very case "syncing" equals to "everything ok"? What would be the other sync statuses?).

Important things first

From that tooltip it looks to me that the most important thing there is the "data is caught up with source", isn't it? I'd put that first and try to summarize it as a single concept: "in sync". However that sounds contradictory with the sync status, which is "syncing". Can we replace that "syncing" with "in sync", and only "syncing" when the progress is less that "100%"?

image

Regarding the 0/128 and 128/128, I'd replace that with progress bars or percentages:

* If 0/128 means nothing to do for "full sync", then 100% there.

* If 128/128 means nothing to do for "incremental sync", then another 100% there.

My guess is that RGW is showing a developer status instead of summarizing that in operator-meaningful terms.

Thanks @epuertat , for the suggestions.

  1. "Status" Badge - since this status is clickable sp turning it into a badge might not hsow it as a clickable link
  2. Reduce info displayed - We have multiple infos to show here such as - status, sync progress and in case of full sync we will be showing the full sync progress as well..so I think having titles can make more sense here
  3. Important things first - Our aim here is to show the whole result that we get from the sync status command..that many users might be familiar with and since we are using the progress bars below so decided to just go with the text oin the tooltip

@aaSharma14
Copy link
Contributor Author

Thanks @aaSharma14 for the great work on this, it's a cool and great addition for sure!

After testing it for a bit, let me give my thoughts:

Looks like it takes a little bit to load, it would be nice to have a loading spinner while the sync status card finishes loading, to let the user know something is going on and it just randomly appears
screen-capture-18.mp4

Regarding of the layout itself, I'm still not a fan of, I think a better placement would have been to have the realm1 -> zg1-realm1 on top of both cards, just below Multisite sync status since the zones below will share a namespace.

Some other things I see:

image-mh

1. I think the naming `Primary Source Zone` is not the most appropiate. Data will flow from the master zone into the rest of the zones in the zonegroup and viceversa, so not sure if this is the best naming for it.

2. Same as before, but a part from that I think if we are naming the first as "source" I don't think the second card should be "source" as well, can be confussing as you would expect  to be a "destination" or something similar

3. This part is confusing to me and kinda out of place to the rest of the layout, I think a better indication that there is anything to sync for the metadata would be appriciated and I'm not sure of what does the `{zone is master}` is supposed to mean

4. No syncing is being made yet the status shows as `syncing`

5. I think the flow of the arrows can be missleading as it kinda looks like the syncing goes from the `realm1` into the `zone1-zg1-realm1` , also I think it pops up too much compared to the rest of the layout when is not one of the main things to be looking at

6. If progress is at 100% I would expect that it said `done`, `completed` or anything similar, to clarify that it has actually finished. This also goes with point 4

7. The text value inside the progress bar should be white as in the other green badges

Thanks @Pegonzal for the review. I agree that we can improve the layout with a better design showing the source zones as a part of the overall realm and zonegroup. I think we can take it as an improvement because of the time constraints.
Regarding the naming convention of the sync statuses - Yes, it is confusing..we can have a call with the rgw team to better understand what they actually mean and if we can show status as per our understanding. Also changed the usage bar text color to white

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Copy link
Member

@nizamial09 nizamial09 left a comment

Choose a reason for hiding this comment

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

I still think there could are rooms for improvements in the 'pythonic' part. But for ts, this is my first impression based on some testing. We could improve it again if the backend data processing is done a little bit more better.. but that's something we can take a look!

also, there are a lot of *ngFor and usage of same variable name in the html. Which is a bit difficult for readability. We should make the code more readable for the future developers who are going to maintain this and for ourselves too!

@aaSharma14 aaSharma14 force-pushed the rgw-multisite-sync-card branch 2 times, most recently from d340c78 to 92c1bee Compare August 16, 2023 11:38
@nizamial09
Copy link
Member

nizamial09 commented Aug 16, 2023

@aaSharma14 angular has in-built pipe titlecase which you can use to titlecase some of the data's that are dumped in the view

@nizamial09
Copy link
Member

also, te loading screen is not working. I have not seen it atleast.

@aaSharma14 aaSharma14 force-pushed the rgw-multisite-sync-card branch 2 times, most recently from 1ce99fc to 3c71576 Compare August 17, 2023 11:14
@aaSharma14 aaSharma14 force-pushed the rgw-multisite-sync-card branch 3 times, most recently from 85db148 to aaad0b3 Compare August 21, 2023 05:24
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
(cherry picked from commit 1d6f19e53b68c180a2d0301889974949fe899a2c)
@aaSharma14
Copy link
Contributor Author

jenkins test make check

@cloudbehl
Copy link
Contributor

jenkins test dashboard

@aaSharma14
Copy link
Contributor Author

jenkins test api

Copy link
Contributor

@Pegonzal Pegonzal left a comment

Choose a reason for hiding this comment

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

Working good, and I'm liking all the new improvments!
LGTM

Copy link
Contributor

@cloudbehl cloudbehl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @aaSharma14

@aaSharma14 aaSharma14 merged commit a4b76ef into ceph:main Aug 21, 2023
11 of 14 checks passed
@aaSharma14 aaSharma14 deleted the rgw-multisite-sync-card branch August 21, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
5 participants