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

[Feature] Add detailed final RPM output #67

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

hawkinsw
Copy link
Member

@hawkinsw hawkinsw commented Jan 7, 2024

Calculate and display additional RPM information when the user requests with the --detailed CLI flag.

@hawkinsw hawkinsw self-assigned this Jan 7, 2024
@moeller0
Copy link

moeller0 commented Jan 8, 2024

I wonder whether this is not also a good opportunity to report the corresponding durations for the different RPM values?
And maybe put the RPM statistics measure at the front, like:

Final detailed RPM ([PNN|ssTRN%]); full: NNN (MMM ms); self: NNN (MMM ms); foreign: NNN (MMM ms)

That would give a concise yet pretty complete detailed report? Moving the stats measure to the front would help in visually finding corresponding lines quicker (at least for me).
The same format could then also be used for the Upload and Download RPM reports if --detailed was requested?

@hawkinsw
Copy link
Member Author

hawkinsw commented Jan 8, 2024

From the Slack channel, there is some additional description from @moeller0 that I will take into account:

Maybe add the respective time unit?

Trimmed Mean Self RTT [sec]:        0.208896

or

Trimmed Mean Self RTT [ms]:        208.896

And, for --detailed we might be able to do something like:

Final RPM(P90): 14 (4285.71 ms); self: 8 (7500.00 ms); foreign: 69 (869.56 ms)
Final RPM(sTR5): 323 (18.58 ms); self: 287 (20.91 ms); foreign: 369 (16.26 ms)

Note the delay numbers are likely wrong... just take this as example...
That is, move the type identifier to the "label", and all all three RPMs in one line and also report the corresponding durations.

@hawkinsw
Copy link
Member Author

hawkinsw commented Jan 9, 2024

@moeller0 Sorry for the late response. I have added additional detailed output -- what you suggested makes the output that much more complete! I am not sure that I matched your suggested formatting. Please let me know what changes you think would be helpful.

One big piece that I did add: The detailed RPM calculation information in the directions. I have no idea why I did not add that before. It's there now and I think that makes the output that much better.

Thank you for the suggestion!

@hawkinsw
Copy link
Member Author

@moeller0 I have updated this branch to produce a slightly more verbose output for trimmed measurements:

	RPM Calculation stats:
		Total Self Probes:            476
		Total Foreign Probes:         1428
		Trimmed Self Probes Count:    23 of 476
		Trimmed Foreign Probes Count: 71 of 1428
		P90 Self RTT:                 0.096804s
		P90 Foreign RTT:              0.033561s
		P90 RTT:                      0.065182s
		Trimmed Mean Self RTT:        0.073682s
		Trimmed Mean Foreign RTT:     0.029493s
		Trimmed Mean RTT:             0.051588s
	RPM: 920 (P90)

Let me know if that looks okay to you!

@hawkinsw hawkinsw force-pushed the final_self_and_foreign_rpms branch 3 times, most recently from b5fd2c7 to 3fa054c Compare January 29, 2024 14:53
Calculate and display additional RPM information when the user requests
with the `--detailed` CLI flag.

Signed-off-by: Will Hawkins <[email protected]>
Add additional detailed output.
Add additional context for trimmed measurements count.
Print out some additional information in detailed mode.
@hawkinsw hawkinsw force-pushed the final_self_and_foreign_rpms branch from 3fa054c to 2483f88 Compare January 29, 2024 15:18
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