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

uniform style of stats/report ascii tables #2084

Merged
merged 1 commit into from
May 2, 2022

Conversation

mgor
Copy link
Contributor

@mgor mgor commented May 2, 2022

I'm using the console statistics tables to get a quick summary of how a test went, and one thing that has annoyed me a bit, is that print_stats, print_percentile_stats and print_error_report ascii tables all have different "style".

Especially print_stats, where method and name is concatenated to one string, resulting in, e.g. GET and POST entries are misaligned.

Off the 3 styles, I liked the print_percentile_stats most, so fixed the other 2 to have the same style.

Comparision with before and after;

print_stats:

# before
 Name                                                                              # reqs      # fails  |     Avg     Min     Max  Median  |   req/s failures/s
----------------------------------------------------------------------------------------------------------------------------------------------------------------
 GET test_entry                                                                       100   20(20.00%)  |      49       0      99      49  |    0.00    0.00
----------------------------------------------------------------------------------------------------------------------------------------------------------------
 Aggregated                                                                           100   20(20.00%)  |      49       0      99      49  |    0.00    0.00

# after
 Type     Name                                                                              # reqs      # fails  |     Avg     Min     Max  Median  |   req/s  failures/s
--------|--------------------------------------------------------------------------------|--------|--------------|--------|-------|-------|---------|--------|------------|
 GET      test_entry                                                                           100   20(20.00%)  |      49       0      99      49  |    0.00        0.00
--------|--------------------------------------------------------------------------------|--------|--------------|--------|-------|-------|---------|--------|------------|
          Aggregated                                                                           100   20(20.00%)  |      49       0      99      49  |    0.00        0.00

print_error_report:

# before
Error report
 # occurrences      Error                                                                                               
----------------------------------------------------------------------------------------------------------------------------------------------------------------
 20                 GET test_entry: RuntimeError('error')                                                               
----------------------------------------------------------------------------------------------------------------------------------------------------------------

# after
Error report
 # occurrences      Error                                                                                               
------------------|---------------------------------------------------------------------------------------------------------------------------------------------|
 20                 GET test_entry: RuntimeError('error')                                                               
------------------|---------------------------------------------------------------------------------------------------------------------------------------------|

The only change in print_percentile_stats is to print an empty string instead of None in the Type column for the Aggregated row:

# before
Response time percentiles (approximated)
 Type     Name                                                                                  50%    66%    75%    80%    90%    95%    98%    99%  99.9% 99.99%   100% # reqs
--------|--------------------------------------------------------------------------------|---------|------|------|------|------|------|------|------|------|------|------|------|
 GET      test_entry                                                                             50     66     75     80     90     95     98     99     99     99     99    100
--------|--------------------------------------------------------------------------------|---------|------|------|------|------|------|------|------|------|------|------|------|
 None     Aggregated                                                                             50     66     75     80     90     95     98     99     99     99     99    100
 
# after
Response time percentiles (approximated)
 Type     Name                                                                                  50%    66%    75%    80%    90%    95%    98%    99%  99.9% 99.99%   100% # reqs
--------|--------------------------------------------------------------------------------|---------|------|------|------|------|------|------|------|------|------|------|------|
 GET      test_entry                                                                             50     66     75     80     90     95     98     99     99     99     99    100
--------|--------------------------------------------------------------------------------|---------|------|------|------|------|------|------|------|------|------|------|------|
          Aggregated                                                                             50     66     75     80     90     95     98     99     99     99     99    100

Added unit tests for all 3 methods.

@heyman
Copy link
Member

heyman commented May 2, 2022

Looks good to me. It's probably a good idea to add an entry to the Changelog, since it's a potentially breaking change when people are parsing the output. For the same reason it might be a good idea to not release this as a "patch" version bump.

@mgor
Copy link
Contributor Author

mgor commented May 2, 2022

Looks good to me. It's probably a good idea to add an entry to the Changelog, since it's a potentially breaking change when people are parsing the output. For the same reason it might be a good idea to not release this as a "patch" version bump.

Fair enough, I kind of assumed that one would use the CSV stats writer if any parsing of the statistics would be done :)

Will you update the changelog when releasing (looks like that's the way its done according to CONTRIBUTING.md (which by the way looks a bit obsolete), or is there something I should update in the PR? I didn't find any instructions under Developing Locust.

@cyberw
Copy link
Collaborator

cyberw commented May 2, 2022

LGTM. Have you checked that it works with resizing the terminal window? I can do the changelog & make sure next version is a minor release rather than patch.

@mgor
Copy link
Contributor Author

mgor commented May 2, 2022

LGTM. Have you checked that it works with resizing the terminal window?

I created a dummy test case, that I will not commit, which prints the statistics every 3 seconds and I resized the terminal between the iterations:
image

Any terminal width smaller than 171 columns/characters will get annoying line wraps, in the old table that should've been 171 - (STATS_TYPE_WIDTH + padding), so something like 162 columns.

image

I can do the changelog & make sure next version is a minor release rather than patch.

Great!

@cyberw cyberw merged commit f4f0487 into locustio:master May 2, 2022
@cyberw
Copy link
Collaborator

cyberw commented May 2, 2022

Nice! I wont make a new releaese just yet, because there is another open PR with potentially breaking changes, but a prerelease build should be published in a couple minutes.

@heyman
Copy link
Member

heyman commented May 2, 2022

I kind of assumed that one would use the CSV stats writer if any parsing of the statistics would be done :)

Yeah, that's probably true, though I remember seeing someone parsing the output at some point, though it might have been quite a long time ago.

Will you update the changelog when releasing (looks like that's the way its done according to CONTRIBUTING.md (which by the way looks a bit obsolete), or is there something I should update in the PR?

I should probably have clarified that I meant the Changelog Highlights in the docs. Sometimes we've previously included it in the PR under a separate Unreleased / In development headline. Then when a release is made it's easy to just move the entry to the correct release.

LGTM. Have you checked that it works with resizing the terminal window? I can do the changelog & make sure next version is a minor release rather than patch.

👍

@mgor mgor deleted the feature/ascii_stats_tables branch May 2, 2022 11:22
@cyberw
Copy link
Collaborator

cyberw commented May 3, 2022

Huh. I only just now saw the full effects of this change. It increases the line length of the print_stats table by ~11 characters, enough to cause line breaks with small terminals and/or long request names. This is kind of an issue...

(which I would have seen in your comment if I hadnt been sloppy and reading it on mobile)

@mgor
Copy link
Contributor Author

mgor commented May 3, 2022

How big of an issue is it really? Sure it's a non-backwards compatible change on request names by something like at max 10 characters (depending on how long the request method is). But the max width is the same as print_percentile_stats was before the change.

STATS_TYPE_WIDTH is 8 characters, not sure about others, but for my custom requests, I usually keep below 6 characters, so might be able to trim some characters there?

Also, since method + name used to be in STATS_NAME_WIDTH, maybe the the Name column could be reduced by STATS_TYPE_WIDTH?

So something like:

def print_stats(stats, current=True):
    console_logger.info(
        (" %-" + str(STATS_TYPE_WIDTH) + "s %-" + str(STATS_NAME_WIDTH - STATS_TYPE_WIDTH) + "s %7s %12s  | %7s %7s %7s %7s  | %7s %11s")
        % ("Type", "Name", "# reqs", "# fails", "Avg", "Min", "Max", "Median", "req/s", "failures/s")
    )

That still leaves a problem with long request names though... but shouldn't be that much of a difference compared to before. As a last restort, an ugly solution would be to substring the names if they're longer than STATS_NAME_WIDTH - STATS_TYPE_WIDTH.

Looking at gnome-terminal, the widest default preset terminal size is 132 columns, which still would be to small even before this change. terminal.app defaults to 80x32.

@cyberw
Copy link
Collaborator

cyberw commented May 4, 2022

Backwards compatibility is not super important, as this is just for human consumption. But lines should be as short as possible, and I think it is higher prio than complete consistency between the tables.

Terminals come in all shapes and sizes these days. My personal limit is "iterm2 on half a macbook pro screen" (so I can have two next to each other :), which is 119 characters.

This used to work for example:
image

but now it becomes:
image

@mgor
Copy link
Contributor Author

mgor commented May 4, 2022

Fair enough. My point with the "default" terminal sizes, was that if the user does not explicitly change it, it would still be too small :)

My suggestion would be to reduce the Name column width with the Type column width (STATS_NAME_WIDTH - STATS_TYPE_WIDTH).

Without the change, minimum width of Name was 80 characters, which would hold type + space + name, which for the normal cases would allow a request name between 76 and 73 characters;
3 + 1 + len(name) (GET <request name>) and 6 + 1 + len(name) (DELETE <request name>)

As it was implemented in this PR, the maximum request name, in the same scenario as above, would always be 80 characters.
But the Type column would always be 8 + 1 wide.
So we're talking about a change of between 2 and 5 characters, but allowing for between 4 and 7 characters more in the request name.

By reducing the Name column width, the request name maximum width (in the minimum case) would be 72 characters, a difference between 1 and 5 characters shorter compared to the original implementation.

But the total width would be 1 character more than the original implementation, which could be saved by removing the ending | in the separators ;) If we also skip the leading space for each entry in the tables (and in headers), we'll save another character.

I can whip up a new PR, and move the discussion there?

@cyberw
Copy link
Collaborator

cyberw commented May 4, 2022

That sounds great, please do! Long request/type names causing line breaks is ok, but it must be possible to avoid by using reasonably short names.

mgor added a commit to mgor/locust that referenced this pull request May 4, 2022
cyberw added a commit that referenced this pull request May 4, 2022
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.

3 participants