-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Fortio (greek for load - load test tool) #445
Conversation
Still wip
Main: - url argument - gather per threads stats and print them Http: - new utility functions for making http calls (only one without 100% test coverage) Periodic: - Factor out thread id calculation in output - don’t use stdout directly, not thread safe - thread index passed to function (so it’s safe to save/use state without locks), always starting at 0 Stats: - Log function that uses the (thread safe) logger for output devel/README.md: - added greek for fortio - my editor removed extra trailing whitespaces
+ make linter happy
fix for connection pooling / running out of socket errors switch from bool debug to int -v verbosity
Semi log fixed buckets
i.e. when we’re falling behind on qps Still todo: merge/aggregation
still todo still is merging (Also some more testing on pXX see TODO)
left to do:
also (but maybe deferred to future PRs):
|
devel/fortio/periodic.go, line 166 at r5 (raw file): Previously, ldemailly (Laurent Demailly) wrote…
ok. i don't think using rate.Limiter is blocking, just wanted to suggest it. I start to get a little nervous when I see custom throttling code, and as you say, getting RPS correct here is a big deal. Comments from Reviewable |
devel/fortio/periodic.go, line 174 at r5 (raw file): Previously, ldemailly (Laurent Demailly) wrote…
I assume you mean #453 ? I'm fine if you want to revisit this in a subsequent, more focused PR. I happen to have a select fetish, and think it (or similar) would make the intent here clearer, but this isn't a blocker. Comments from Reviewable |
devel/fortio/periodic.go, line 174 at r5 (raw file): yes. though I'd still need to be able to do 40kps on my mac to make the change Comments from Reviewable |
devel/fortio/periodic.go, line 81 at r7 (raw file): Previously, ldemailly (Laurent Demailly) wrote…
Done. Comments from Reviewable |
devel/fortio/periodic.go, line 119 at r5 (raw file): Previously, douglas-reid (Douglas Reid) wrote…
Comments from Reviewable |
devel/fortio/README.md, line 17 at r5 (raw file): Previously, ldemailly (Laurent Demailly) wrote…
no great suggestions, unfortunately. just worried about potential user surprise or questions. Comments from Reviewable |
devel/fortio/stats.go, line 126 at r5 (raw file): Previously, ldemailly (Laurent Demailly) wrote…
no rule of thumb; up to you. Comments from Reviewable |
devel/fortio/README.md, line 17 at r5 (raw file): Previously, douglas-reid (Douglas Reid) wrote…
np. will update doc as questions come (and also probably the README / godoc). thx Comments from Reviewable |
devel/fortio/periodic.go, line 29 at r5 (raw file): Previously, ldemailly (Laurent Demailly) wrote…
I'm struggling to come up with a better name -- Function just seems too generic. But I won't block, as this isn't important. Comments from Reviewable |
devel/fortio/http.go, line 15 at r5 (raw file): Previously, ldemailly (Laurent Demailly) wrote…
as far as I can tell, this layout address my concerns. Comments from Reviewable |
devel/fortio/periodic.go, line 29 at r5 (raw file): Previously, douglas-reid (Douglas Reid) wrote…
thx ! btw it's not a keyword, and scoped to a package and a type so it's not that generic Comments from Reviewable |
Reviewed 1 of 20 files at r1, 16 of 23 files at r6, 1 of 2 files at r9, 2 of 2 files at r10, 4 of 4 files at r11. Comments from Reviewable |
devel/fortio/http.go, line 15 at r5 (raw file): Previously, douglas-reid (Douglas Reid) wrote…
thx! Comments from Reviewable |
Cool. I look forward to seeing the regression suite integration. I think all of my high-level concerns have been addressed or answered. I look forward to the mentioned improvements. Thanks for your patience in getting this review done. Comments from Reviewable |
thanks a lot for all the thoughtful comments and you patience in educating me about go ! greatly appreciated! can you accept it in github too ? tia Review status: all files reviewed at latest revision, 34 unresolved discussions. Comments from Reviewable |
Jenkins job istio/presubmit passed |
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.
My concerns have been addressed.
Reviewed 1 of 20 files at r1, 16 of 23 files at r6, 1 of 2 files at r9, 2 of 2 files at r10, 4 of 4 files at r11. Comments from Reviewable |
Reviewed 1 of 23 files at r6, 1 of 1 files at r12. Comments from Reviewable |
Jenkins job istio/presubmit passed |
1 similar comment
Jenkins job istio/presubmit passed |
* Fortio (greek for load - load test tool) (cmd/fortio/main.go): - url argument - gather per threads stats and print them Http: - new utility functions for making http calls (only one without 100% test coverage) Periodic: - Factor out thread id calculation in output - don’t use stdout directly, not thread safe - thread index passed to function (so it’s safe to save/use state without locks), always starting at 0 Stats: - Log function that uses the (thread safe) logger for output devel/README.md: - added greek for fortio - my editor removed extra trailing whitespaces * Debug dumps full headers * use -c for simulataneous connections / 'threads' fix for connection pooling / running out of socket errors switch from bool debug to int -v verbosity * Adding Histogram Semi log fixed buckets * Calculate % of negative sleep time i.e. when we’re falling behind on qps * Adding test for first and last buckets * Updated defaults + nicer usage string and version * Supports multiple -H for additional Headers * Host header is special * Go ignores Host header with spaces (!) * Validate extra headers early + more efficient + trim * Now aggregating histograms after the threads are done Support for .Clone() .Transfer() .Reset() .CopyFrom() in Counter/Histogram Aggregate data Reduced default verbosity (use -v 1 to see per thread stats and body sizes histogram) * Added -p for list of percentiles, -r for resolution... Added -p for list of percentiles, -r for resolution. Also added very first rough cut at some doc/README * Even better when actually using the resolution arg Added a more meaty example, will add picture later * adding a picture * New maxqps mode (-qps 0) and low overhead echo server IPeriodicRunner w Setter/Getter -> just NewPeriodicRunner(params *RunnerOptions) thanks Mandar and Doug for the hint ! Also pass the r pointer for most things in runOne() Also added tests for maxqps and parse percentiles. Coverage 81% (missing are mostly fatals and verbose > 1) * Code review changes Thanks a lot Doug and Jason for all the comments * Moved files to symmetrical layout library at top level and command lines under cmd/ If we split into multiple packages we can move it under fortio/pkg/fortio but that seems ugly right now [2x fortio in the path, though it’s there for cmd/fortio/…]) * Improve runOne param name/self doc * Factor the defaults out per code review suggestions <rant>wish there was const for struct (and pointers)</rant> * Fixed readme links Thx for the review * code reviews comments + use glog in echosrv * Further (last ;-) ?) set of code review suggestions (godoc, Options(), if...) * Dropping last defer Former-commit-id: 2bcb4dc
* Fortio (greek for load - load test tool) (cmd/fortio/main.go): - url argument - gather per threads stats and print them Http: - new utility functions for making http calls (only one without 100% test coverage) Periodic: - Factor out thread id calculation in output - don’t use stdout directly, not thread safe - thread index passed to function (so it’s safe to save/use state without locks), always starting at 0 Stats: - Log function that uses the (thread safe) logger for output devel/README.md: - added greek for fortio - my editor removed extra trailing whitespaces * Debug dumps full headers * use -c for simulataneous connections / 'threads' fix for connection pooling / running out of socket errors switch from bool debug to int -v verbosity * Adding Histogram Semi log fixed buckets * Calculate % of negative sleep time i.e. when we’re falling behind on qps * Adding test for first and last buckets * Updated defaults + nicer usage string and version * Supports multiple -H for additional Headers * Host header is special * Go ignores Host header with spaces (!) * Validate extra headers early + more efficient + trim * Now aggregating histograms after the threads are done Support for .Clone() .Transfer() .Reset() .CopyFrom() in Counter/Histogram Aggregate data Reduced default verbosity (use -v 1 to see per thread stats and body sizes histogram) * Added -p for list of percentiles, -r for resolution... Added -p for list of percentiles, -r for resolution. Also added very first rough cut at some doc/README * Even better when actually using the resolution arg Added a more meaty example, will add picture later * adding a picture * New maxqps mode (-qps 0) and low overhead echo server IPeriodicRunner w Setter/Getter -> just NewPeriodicRunner(params *RunnerOptions) thanks Mandar and Doug for the hint ! Also pass the r pointer for most things in runOne() Also added tests for maxqps and parse percentiles. Coverage 81% (missing are mostly fatals and verbose > 1) * Code review changes Thanks a lot Doug and Jason for all the comments * Moved files to symmetrical layout library at top level and command lines under cmd/ If we split into multiple packages we can move it under fortio/pkg/fortio but that seems ugly right now [2x fortio in the path, though it’s there for cmd/fortio/…]) * Improve runOne param name/self doc * Factor the defaults out per code review suggestions <rant>wish there was const for struct (and pointers)</rant> * Fixed readme links Thx for the review * code reviews comments + use glog in echosrv * Further (last ;-) ?) set of code review suggestions (godoc, Options(), if...) * Dropping last defer Former-commit-id: 2bcb4dc
istio#445) * networking: Restrict port match in match condition to use port numbers only Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * Clarify semantics of hostnames, ports, etc Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * nits Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * nits Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
* Remove disabled components when running mesh manifest apply. * Add workarounds for pruning component when getting empty manifests. * Fix GetAll() func comment. * Fix status update logic in reconciler when manifests are empty. * Update with some comments and catch an edge case. * Add tests for kubectl Delete() and GetAll(). * Refactor client.go to extract the common code of kubectl execution. * Add comments for kubectlParams, and improve comments for kubectl(). * Polish the log and comments. * Remove redundant comments on kubectlParams.
Command line help:
Example output:
This change is