Abort ping test if pings are too long #1872
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1872 +/- ##
============================================
- Coverage 36.36% 36.32% -0.05%
Complexity 18780 18780
============================================
Files 1081 1081
Lines 166364 166557 +193
Branches 35399 35431 +32
============================================
+ Hits 60506 60507 +1
- Misses 95973 96165 +192
Partials 9885 9885
Continue to review full report in Codecov by Sentry.
|
jaadams5
approved these changes
Aug 4, 2023
Did you try it out? I'll merge it and you can see for yourself. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Shiverwarp complained recently that he was getting login ping test times of 23000+ msec and it was taking seemingly forever to login. I sympathize; I usually get <30 msec and the longest I've see in a 10-ping test is a bit more than 800 - which would still be an intolerably slow connection. And to think it took 10 pings to get that "average".
I can't imagine what caused Shiverwarp's insanely bad connection, but if it took 4 minutes to run the first test and decide it is time to retry, seems to me that we could recognize way earlier - as early as the very first ping - that this connection is a no-go.
Comparing ping average times with goals and thresholds in floating point seems right, but displaying them as integers, calculated either by Math.round() or Math.floor(), is problematic. If your threshold calculation says you cannot exceed 30.2 msec and you generate 30.4, it will fail, and using Math.round, will report "you wanted 30 msec but got 30 msec, so it failed". I changed it to display floating point results in floating point. So far, it always seems to display with a single digit to the right of the decimal point, which seems fine.
If you get a really slow connection, it will be obvious early on that it will be too slow.
Allow configuration for "PingAbortTrigger" triggers.
If "pingTestAbort" has "4:3|1:10", you are specifying:
Retry if 4 pings exceed 3 times average historical ping
Retry if 1 ping exceeds 10 times average historical ping
Default is "1:10"
On the PingOptionsPanel, you can add rows to specify additional abort triggers or delete rows to remove them. If you try to delete the last row, it just zeroes it - which will leave the property empty.
I think the GUI is pretty spiffy.
PingManager now checks those triggers after each ping it submits. If that results in any trigger firing - 4 exceeding 3 times historical average or 1 exceeding 10 times historical average, from the above example - PingManager aborts the ping test and returns the test with the trigger that fired to LoginManager.
Could be that the aborted ping test was still good enough, but if not, it reports that the ping test aborted and it moves on to the next timein attempt.