-
Notifications
You must be signed in to change notification settings - Fork 208
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
Connectivity Test: Add latency measurement #2094
Conversation
105981a
to
d6e81f8
Compare
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.
One suggestion here is to add the ability to see percentile results in addition to the average. Having 50, 90, and 99th percentiles can be really helpful when trying to understand the average.
d6e81f8
to
d074005
Compare
d074005
to
12bf3eb
Compare
Implemented |
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.
Great! Thank you for this. I have one nit, but otherwise LGTM.
Also, just a note, averaging percentiles can sometimes get you into trouble (see https://www.circonus.com/2018/11/the-problem-with-percentiles-aggregation-brings-aggravation/). However, I think that for this use case it's totally valid. The percentiles you are averaging together are samples from the same test, so the distributions should be the same.
12bf3eb
to
e66d6eb
Compare
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.
@darox Nice work!
e66d6eb
to
9e62b69
Compare
Thank you. I implemented all of your proposals. |
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.
LGTM. It might make sense at some point to consider collapsing all of the --perf-*
flags into one --perf
flag that accepts enums such as --perf=latency
or --perf=net
. The former runs latency tests and the latter only runs the base "network" tests. We could have --perf=all
to run all perf-related tests. This would reduce the need to keep adding more and more flags. Not required for this PR though.
Thank you for the feedback. Absolutely, this is something I wanna work on after this PR is merged. |
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.
LGTM. One non-blocking nit regarding summary headers for latency tests vs. existing performance tests. Can be addressed as a follow-up. Thanks.
Awesome catch. I didn't see that. |
Add netperf based latency tests that can be triggered by running `cilium connectivity test --perf --perf-latency --perf-samples 5` Signed-off-by: Dario Mader <[email protected]>
a2fa881
to
e3d1ec7
Compare
This PR adds netperf based latency tests that can be triggered by running: