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

HDDS-10864. Recon Disk Usage If one volume is large it occupies almost all space of Pie Chart #6801

Merged
merged 13 commits into from
Jun 28, 2024

Conversation

smitajoshi12
Copy link
Contributor

@smitajoshi12 smitajoshi12 commented Jun 10, 2024

What changes were proposed in this pull request?

Recon Disk Usage if one entity is large it occupies almost all space and other entities are not visible in Pie Chart.

Please describe your PR in detail:
file1 -> Size -> 1 KB
file2 -> Size -> 10 KB
file3 -> Size -> 1 GB
The API endpoint would return a response in descending order of size. However, the problem is that the UI representation becomes skewed, as shown in the image below:
Here, we have three directories with sizes 1 KB, 10 KB, and 1 GB. I believe the size of each part of the pie chart is relative to the file size, but this creates a poor user experience. We need to address this issue to improve the user interface.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10864

How was this patch tested?

Manually

Case 1
Total :- 1000011010,
Vol1:- 1000 :- 1 KB
vol2:- 10000 :- 10 KB
vol3:- 1000000000 :- 1GB
vol4:- 10 :- 10 Bytes

Before this PR

image

After this patch

image

Case 2

Total :- 125000010:- 125 MB
vol1:-5MB :- 5000000
vol2 :-20MB:- 20000000
vol3:- 100MB:- 100000000
vol4:- 10KB :- 10

{
"status": "OK",
"path": "/",
"size": 125000010,
"sizeWithReplica": -1,
"subPathCount": 5,
"subPaths": [
{
"key": false,
"path": "/vol1",
"size": 5000000,
"sizeWithReplica": -1,
"isKey": false
},
{
"key": false,
"path": "/vol-2",
"size": 20000000,
"sizeWithReplica": -1,
"isKey": false
},
{
"key": false,
"path": "/vol3",
"size": 100000000,
"sizeWithReplica": -1,
"isKey": false
},
{
"key": false,
"path": "/vol4",
"size": 10,
"sizeWithReplica": -1,
"isKey": false
}
],
"sizeDirectKey": -1
}

Before this PR

image

After this patch

image-

Case 3

Total :- 501250000 :- 501.25MB
vol1-50KB :- 50000
vol2-200KB:- 200000
vol3-1MB:- 1000000
vol4-500MB:- 500000000

{
"status": "OK",
"path": "/",
"size": 501250000,
"sizeWithReplica": -1,
"subPathCount": 5,
"subPaths": [
{
"key": false,
"path": "/vol1",
"size": 50000,
"sizeWithReplica": -1,
"isKey": false
},
{
"key": false,
"path": "/vol-2",
"size": 2000000,
"sizeWithReplica": -1,
"isKey": false
},
{
"key": false,
"path": "/vol3",
"size": 1000000,
"sizeWithReplica": -1,
"isKey": false
},
{
"key": false,
"path": "/vol4",
"size": 500000000,
"sizeWithReplica": -1,
"isKey": false
}
],
"sizeDirectKey": -1
}

Before this PR

image

After this patch

image

Cluster Testing

Screenshot 2024-06-12 at 6 23 45 PM

image

Zero Size Volumes Cluster Testing
{
"status": "OK",
"path": "/",
"size": 17289,
"sizeWithReplica": -1,
"subPathCount": 3,
"subPaths": [
{
"key": false,
"path": "/vol",
"size": 17289,
"sizeWithReplica": -1,
"isKey": false
},
{
"key": false,
"path": "/s3v",
"size": 0,
"sizeWithReplica": -1,
"isKey": false
},
{
"key": false,
"path": "/volume",
"size": 0,
"sizeWithReplica": -1,
"isKey": false
}
],
"sizeDirectKey": -1
}

With this PR

image

Tooltip

image-

Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 left a comment

Choose a reason for hiding this comment

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

Thank's a lot for working on this patch, @smitajoshi12. I have a few comments, but overall, the patch looks good.

@ArafatKhan2198
Copy link
Contributor

@smitajoshi12
Could you please test the normalization feature with the following scenarios to ensure it provides a balanced visual representation in the pie chart?

  1. Scenario 1:

    • file1: 1 KB
    • file2: 10 KB
    • file3: 1 GB
  2. Scenario 2:

    • file1: 5 MB
    • file2: 20 MB
    • file3: 100 MB
  3. Scenario 3:

    • file1: 50 KB
    • file2: 200 KB
    • file3: 1 MB
    • file4: 500 MB

These tests will help verify that the normalization logic works well across different file size distributions.

@smitajoshi12
Copy link
Contributor Author

@smitajoshi12 Could you please test the normalization feature with the following scenarios to ensure it provides a balanced visual representation in the pie chart?

  1. Scenario 1:

    • file1: 1 KB
    • file2: 10 KB
    • file3: 1 GB
  2. Scenario 2:

    • file1: 5 MB
    • file2: 20 MB
    • file3: 100 MB
  3. Scenario 3:

    • file1: 50 KB
    • file2: 200 KB
    • file3: 1 MB
    • file4: 500 MB

These tests will help verify that the normalization logic works well across different file size distributions.

@ArafatKhan2198
Added screenshots with above file sizes.

@ArafatKhan2198
Copy link
Contributor

@smitajoshi12 Did you test out with zero sized buckets or volumes?

@smitajoshi12
Copy link
Contributor Author

@smitajoshi12 Did you test out with zero sized buckets or volumes?

@ArafatKhan2198
Attached latest screenshots

Copy link
Contributor

@dombizita dombizita left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @smitajoshi12! I'm not sure overall about this change. I know that the current behaviour is also not the best, but I believe with this change it can be confusing for the users as well.
I think by adding the MIN_BLOCK_SIZE to all the values can be a bad idea, as e.g. when you have zero sized volumes/buckets, it's weird that they take up any space from the pie chart.
I don't know what could be a good solution, but with the attached screenshots (they are very useful, thanks!) I think in some of the cases the pie charts are confusing, because if you compare the sizes, they are very different, but from the pie charts you may not think that.
Anyone else any ideas? Maybe some good practices around pie charts?

@dombizita
Copy link
Contributor

dombizita commented Jun 18, 2024

Ohh, wait a minute. Based on the npm doc you attached: Normalize array to unit length, that is 0..1 range. Any linear normalisation won't change the size of the slices in the pie chart... So overall with this change you are just adding that number (MIN_BLOCK_SIZE) to every slice and show it like that... I don't think this is a good solution.
I think if we don't like that there are huge differences in the volume/bucket sizes, we should rather change the chart we are using.

@smitajoshi12
Copy link
Contributor Author

Ohh, wait a minute. Based on the npm doc you attached: Normalize array to unit length, that is 0..1 range. Any linear normalisation won't change the size of the slices in the pie chart... So overall with this change you are just adding that number (MIN_BLOCK_SIZE) to every slice and show it like that... I don't think this is a good solution. I think if we don't like that there are huge differences in the volume/bucket sizes, we should rather change the chart we are using.

@zita We are not changing percentage or any other logic just adding extra slice space on entities we will have max 30 entities we can accomodate accordingly easily.

@dombizita
Copy link
Contributor

I'd suggest to definitely let the users know, that in favour to have a small slice for the every entity (even if its size is very small) there is a normalisation of the sizes (actually we normalise the entity sizes and add a small amount to all of them, to achieve that all of them are visible in the pie chart). To have an entity's proper size and percentage of the total we can add something, I think that is also needed. I think if an entity has 0 size we should not show it, otherwise it'd be confusing. So overall:

  • Add some information to the user, what we do in the pie chart calculation.
  • Add information about the real size and percentage of every entity (beside the calculated values, that are default visible on the pie chart)
  • If an entity's size is 0, we should not add that minimum block amount to it and we should just simply not show it in the pie chart, like before.

@smitajoshi12
Copy link
Contributor Author

I'd suggest to definitely let the users know, that in favour to have a small slice for the every entity (even if its size is very small) there is a normalisation of the sizes (actually we normalise the entity sizes and add a small amount to all of them, to achieve that all of them are visible in the pie chart). To have an entity's proper size and percentage of the total we can add something, I think that is also needed. I think if an entity has 0 size we should not show it, otherwise it'd be confusing. So overall:

  • Add some information to the user, what we do in the pie chart calculation.
    @dombizita Added tooltip screenshot but you can suggest appropriate text in tooltip.
  • Add information about the real size and percentage of every entity (beside the calculated values, that are default visible on the pie chart)

As we are showing real size and perecntage on pi chart for every entity. Values [] we are using for only space allocation in pi chart there is no visibility anywhere.

  • If an entity's size is 0, we should not add that minimum block amount to it and we should just simply not show it in the pie chart, like before.

handled the above logic with this condition.
adddedvaluesBlockSize = clonedValues && clonedValues.map((item: number) => item > 0 ? item + MIN_BLOCK_SIZE : item);

@dombizita @ArafatKhan2198
I have removed external dependency from code not sure about any transitive dependecy will come in future or upgradation of library or license issue may occure for any sub library dependency graph. Used same logic with existing values so we can easily implement with Echarts library suggested by abhishek.

@smitajoshi12 smitajoshi12 requested a review from dombizita June 25, 2024 07:07
Copy link
Contributor

@devabhishekpal devabhishekpal left a comment

Choose a reason for hiding this comment

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

This change looks good to me.
Please do take a look after the other changes requested by @ArafatKhan2198 and @dombizita
Thanks.

@smitajoshi12
Copy link
Contributor Author

This change looks good to me. Please do take a look after the other changes requested by @ArafatKhan2198 and @dombizita Thanks.

It is already fixed all changes suggested by arafat and Zita.

Copy link
Contributor

@dombizita dombizita left a comment

Choose a reason for hiding this comment

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

Thanks for your continuous effort on this @smitajoshi12! Just two small comments, after that I'm fine with the changes. Please update the screenshots in the PR description!

@dombizita
Copy link
Contributor

@ArafatKhan2198 could check on this? Thanks!

Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @smitajoshi12
LGTM +1

@ArafatKhan2198
Copy link
Contributor

Thanks for the patch @smitajoshi12 & thanks for the review @dombizita @devabhishekpal

@ArafatKhan2198 ArafatKhan2198 merged commit 7df3120 into apache:master Jun 28, 2024
33 checks passed
@smitajoshi12 smitajoshi12 deleted the HDDS-10864 branch July 25, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants