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

Customization Feature for Percentile Display on Statistics Page #2550

Merged
merged 25 commits into from Jan 16, 2024

Conversation

FooQoo
Copy link
Contributor

@FooQoo FooQoo commented Jan 14, 2024

Overview

This PR introduces a feature that allows users to freely specify which percentiles are displayed on the statistics page of Locust. Additionally, minor modifications are made to align the display content of the statistics page with that of the charts page.

Implementation Details

  1. A new variable PERCENTILES_TO_STATISTICS is added, enabling users to specify the percentiles displayed on the statistics page.
  2. The specified values in PERCENTILES_TO_STATISTICS will be reflected in both the existing UI and the modern UI of Locust.
  3. Add the column control feature for the modern UI.

Display Modifications

  • Changed "Median (ms)" to "50%ile (ms)".
  • Changed "90%ile (ms)" to "95%ile (ms)".

Background

Based on Issue #2546, this enhancement aims to improve the customizability of the statistics page and ensure consistency with the charts page. This will enable users to analyze data more effectively.

@cyberw
Copy link
Collaborator

cyberw commented Jan 14, 2024

I was thinking this could be even nicer to have controlled in the UI at run time?

Is _TO_STATICS an abbreviation of _TO_STATISTICS? Looks weird to my eyes :)

Median is more easily understood by people than 50%ile. Although tbh, I think we might want to remove it from the default anyways, so maybe it doesnt matter and we could just choose whatever is less complex code.

Build fails because of code formatting.

What do you think @andrewbaldwin44 ?

@cyberw
Copy link
Collaborator

cyberw commented Jan 14, 2024

Does the test actually cover the use of PERCENTILES_TO_STATICS?

@FooQoo FooQoo changed the title Customization Feature for Percentile Display on Statics Page Customization Feature for Percentile Display on Statistics Page Jan 15, 2024
@andrewbaldwin44
Copy link
Collaborator

Hey looking good! I also had the thought (and I'm not sure if this is what you meant @cyberw) that it could be nice to be able to control the columns in the web UI. Something like this:
image

The nice part about PERCENTILES_TO_STATICS is that it allows users to set the percentiles they care about and potentially saves some performance

locust/web.py Outdated Show resolved Hide resolved
@andrewbaldwin44
Copy link
Collaborator

andrewbaldwin44 commented Jan 15, 2024

Yeah maybe we can add some tests for PERCENTILES_TO_STATISTICS (at least in the backend) and some validation to ensure that the statistics provided are valid? Similar to what we do with PERCENTILES_TO_CHART

@FooQoo
Copy link
Contributor Author

FooQoo commented Jan 15, 2024

@cyberw @andrewbaldwin44

Thank you for your positive feedback and insightful suggestions. Here is my response to the points you have raised:

Addressed Items

  1. Concern About Variable Name and Build Failure: These issues have already been addressed in a recent commit. I corrected the variable name from _TO_STATICS to _TO_STATISTICS and resolved the build failure issue by applying the ruff formatter.

Next Actions

  1. Test Coverage for PERCENTILES_TO_STATISTICS: I plan to add tests to cover this new feature to ensure its proper functioning and integration.

Regarding Your Suggestions

  1. On 'Median' vs '50%ile' Clarity: Thank you for your valuable opinion. I am considering keeping 'Median' as the default statistical measure and not including '50%ile' in the percentiles. This aligns with your suggestion and I believe it will make the statistics more user-friendly.

  2. Column Control in Web UI: I will try implementing the proposed mechanism that allows users to control the display columns directly in the Web UI.

Additional Question

Regarding the support for the legacy UI, I have already implemented some features. Should I consider removing these to focus solely on the modern UI, or would it be more beneficial to maintain them? Your guidance on this matter would be greatly appreciated.

@FooQoo
Copy link
Contributor Author

FooQoo commented Jan 15, 2024

Added unit tests for normalize_decimal and integration tests, also reverted percentile display from 50%ile back to median.

@@ -203,4 +203,6 @@ The list of statistics parameters that can be modified is:
+-------------------------------------------+--------------------------------------------------------------------------------------+
| PERCENTILES_TO_CHART | The list of response time percentiles for response time chart |
+-------------------------------------------+--------------------------------------------------------------------------------------+
| PERCENTILES_TO_STATISTICS | The list of response time percentiles for response time statistics |
Copy link
Collaborator

@cyberw cyberw Jan 15, 2024

Choose a reason for hiding this comment

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

Suggest you change this to "list of response time percentiles to show in the statistics table"

And maybe remove "The" from all descriptions, I dont know why that is in there :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

instead maybe add something about this and PERCENTILES_TO_CHART being UI settings (just to make it clear)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cyberw
I've updated the descriptions for PERCENTILES_TO_CHART and PERCENTILES_TO_STATISTICS as suggested.
3981277

@andrewbaldwin44
Copy link
Collaborator

In main.py, we have some validation to ensure the provided PERCENTILES_TO_CHART is correct:

# line 133
for percentile in stats.PERCENTILES_TO_CHART:
        if not is_valid_percentile(percentile):
            logging.error(
                "stats.PERCENTILES_TO_CHART parameter need to be float and value between. 0 < percentile < 1 Eg 0.95\n"
            )
            sys.exit(1)

Perhaps we should have this same validation for PERCENTILES_TO_STATISTICS?

@FooQoo
Copy link
Contributor Author

FooQoo commented Jan 15, 2024

Added a Column Control feature to the Web UI in a PR. Would love your feedback on it!

image

@andrewbaldwin44
Copy link
Collaborator

Awesome work with the column selector! The code and the tests look really great! :) Just a suggestion - what if we had the useSelectViewColumns hook return a new table structure with the hidden columns filtered out? This way we wouldn't have to change the table component

@FooQoo
Copy link
Contributor Author

FooQoo commented Jan 16, 2024

@andrewbaldwin44
Thanks for the feedback and the suggestion!
I've updated the useSelectViewColumns hook to return a new table structure.

I've updated the useSelectViewColumns hook to include the filterStructure function.

Now it takes a structure argument and returns a new table structure with only the visible columns, filtering out the hidden ones.
This should indeed help us avoid modifying the table component. Let me know what you think!

6458cde
c214bec

@andrewbaldwin44
Copy link
Collaborator

Looks awesome, nice work! 💯 @cyberw Good with you to merge?

@cyberw cyberw merged commit d5b3202 into locustio:master Jan 16, 2024
14 checks passed
@FooQoo FooQoo deleted the feature/user-editable-statics-columns branch January 16, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants