-
Notifications
You must be signed in to change notification settings - Fork 14
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
Integrate packet-test #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wait for approval from @robertodauria also
Reviewed 6 of 6 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @cristinaleonr and @robertodauria)
app/measure/measure.js
line 59 at r1 (raw file):
} await runPT(sessionID)
Just clarifying - this is synchronous from the client's perspective? - if so, and if it runs slowly for any reason, the user would experience a delay before seeing the results from the earlier tests? What's the maximum delay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment about randomizing the order of execution. Also, some visual indication that a test is happening would be good -- this is probably more relevant if the order of execution is randomized, since right now the user just sees a delay before the results table appears, but after randomizing the delay might end up at the beginning or between ndt7/msak tests.
Reviewed 5 of 6 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @cristinaleonr)
app/measure/measure.js
line 59 at r1 (raw file):
} await runPT(sessionID)
I believe the order of execution should be randomized in the same way as ndt7/msak. The rationale behind the randomization is that a link where data has been transferred up to half a second ago may exhibit different behavior compared to a link that hasn't seen any significant usage in a while, so any pre-defined order might bias the results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you both. PTAL my latest comment.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @robertodauria and @stephen-soltesz)
app/measure/measure.js
line 59 at r1 (raw file):
Previously, stephen-soltesz (Stephen Soltesz) wrote…
Just clarifying - this is synchronous from the client's perspective? - if so, and if it runs slowly for any reason, the user would experience a delay before seeing the results from the earlier tests? What's the maximum delay?
This is a good point. I've made it async so there is no user delay/impact.
app/measure/measure.js
line 59 at r1 (raw file):
Previously, robertodauria (Roberto D'Auria) wrote…
I believe the order of execution should be randomized in the same way as ndt7/msak. The rationale behind the randomization is that a link where data has been transferred up to half a second ago may exhibit different behavior compared to a link that hasn't seen any significant usage in a while, so any pre-defined order might bias the results.
I agree a randomized order would be ideal. That said, now that it's async it would have to go last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @stephen-soltesz)
app/measure/measure.js
line 59 at r1 (raw file):
Previously, cristinaleonr (Cristina Leon) wrote…
I agree a randomized order would be ideal. That said, now that it's async it would have to go last.
If scientific accuracy is a concern, I still think this should be randomized, with some visual indication that a test is happening (no need for a percentage or anything, perhaps something like "Running short NDT" since it's just a couple of seconds?) so it can be synchronous and go between existing tests.
I'm not willing to block this PR on that, but noting that this is a very easy criticism to make to any data collected this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @stephen-soltesz)
app/measure/measure.js
line 59 at r1 (raw file):
Previously, robertodauria (Roberto D'Auria) wrote…
If scientific accuracy is a concern, I still think this should be randomized, with some visual indication that a test is happening (no need for a percentage or anything, perhaps something like "Running short NDT" since it's just a couple of seconds?) so it can be synchronous and go between existing tests.
I'm not willing to block this PR on that, but noting that this is a very easy criticism to make to any data collected this way.
Indeed, scientific accuracy is a concern. From the conversations we've been having, my understanding is that user impact is a predominant consideration. Making it go last avoids user disruption and at least makes any bias consistent.
This PR updates the client to run a BBR-terminated ndt7 download test unconditionally after the full-length nd7 and MSAK tests are run. The test only takes a couple seconds to complete. The start of the test and the results are logged.
The PR also updates the website language to state that multiple tests are being run.
The data test data is in BigQuery.
The sandbox preview is available here.
This change is