Introduce a perf testing framework for Mixer.#1931
Introduce a perf testing framework for Mixer.#1931ozevren merged 6 commits intoistio:masterfrom ozevren:perf
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:You can indicate your approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #1931 +/- ##
==========================================
- Coverage 83.87% 81.83% -2.05%
==========================================
Files 145 218 +73
Lines 10095 17388 +7293
==========================================
+ Hits 8467 14229 +5762
- Misses 1387 2650 +1263
- Partials 241 509 +268
Continue to review full report at Codecov.
|
mixer/pkg/perf/client.go
Outdated
There was a problem hiding this comment.
Consider calling this Close and passing back the return value of c.conn.Close()
mixer/pkg/perf/client.go
Outdated
There was a problem hiding this comment.
I think we need to track error codes here. Otherwise, some misconfiguration or other problem could lead to totally incorrect perf measurements.
mixer/pkg/perf/clientserver.go
Outdated
There was a problem hiding this comment.
An "RPC rpcServer"? Feels like a search-and-replace error...
mixer/pkg/perf/clientserver.go
Outdated
There was a problem hiding this comment.
Combine with previous line.
mixer/pkg/perf/clientserver.go
Outdated
There was a problem hiding this comment.
Combine with previous line.
mixer/pkg/perf/server.go
Outdated
There was a problem hiding this comment.
Combine with previous line.
mixer/pkg/perf/server.go
Outdated
There was a problem hiding this comment.
Combine with previous line.
Failure paths below should clean up this directory.
There was a problem hiding this comment.
I started out that way, but later decided against it to help debuggability.
mixer/pkg/perf/server.go
Outdated
There was a problem hiding this comment.
Combine with previous line.
mixer/pkg/perf/setup.go
Outdated
There was a problem hiding this comment.
Might be worthwhile to express "environment variable" differently, since it's not OS env vars, right?
mixer/pkg/perf/setup.go
Outdated
|
subscribe |
guptasu
left a comment
There was a problem hiding this comment.
Looks good. Minor comments.
mixer/pkg/mock/client.go
Outdated
mixer/pkg/perf/client.go
Outdated
There was a problem hiding this comment.
The content of this func suggest that we can simply rename this func to func NewClient(mixsAddr string, setup *Setup) (*client, err) {...}
There was a problem hiding this comment.
Not sure what you mean by that? Initialize matches to the incoming initialize call from the controller. I think keeping them aligned makes sense.
There was a problem hiding this comment.
The current model is to first construct using client{}, then initialize and then run, which can be compressed into just two steps of NewClient and Run
mixer/pkg/perf/client.go
Outdated
There was a problem hiding this comment.
nit: the comment is not adding much value, IMO
mixer/pkg/perf/client.go
Outdated
There was a problem hiding this comment.
Consider passing only the thing that the client needs. the Config has a lot of stuff that is meant for Server. Consider making a another ClientConfig struct and converting the Setup into into what is relevant for the client.
There was a problem hiding this comment.
I think that is too costly. Having a single, format keeps things consistent and avoid excessive refactoring cost when we need to change the main data structure (i.e. Setup).
mixer/pkg/perf/clientserver.go
Outdated
There was a problem hiding this comment.
Do we need to call shutdown on the underlying Mixer client as well ?
mixer/pkg/perf/controller_test.go
Outdated
There was a problem hiding this comment.
It will be awesome if the controller takes a config of all the Clients and just inits it for me and waits etc...
From users point of view, this model has two ways to trigger runClient, one by invoking runClients on the controller and other by directly executing the Run method on the NewClientServer object.
There was a problem hiding this comment.
Controller is meant to be internal only. The only reason it is "public" is because the RPC framework requires it. The only supported way to execute things is to call the "Run*" methods.
mixer/pkg/perf/controller_test.go
Outdated
There was a problem hiding this comment.
For now, should we just add a function on the controller, "AddClient(name string) and let the controller manager the wait etc for me and I don't need to know there exists a NewClientServer
There was a problem hiding this comment.
Currently, such logic is located in the "Run" methods. In the future, we can refactor and make the Controller to be a bit more logic-heavy, with figuring out how to launch the clients themselves as well. However, I want to avoid checking a bare minimum infrastructure first, and make improvements on it later.
mixer/pkg/perf/load.go
Outdated
There was a problem hiding this comment.
Consider renaming to Marshal and Unmarshal. Json is just implementation detail
There was a problem hiding this comment.
These are interface methods, used by the JSON marshaller. I can't change them.
mixer/test/perf/perfclient/main.go
Outdated
mixer/test/perf/perfclient/main.go
Outdated
|
@ozevren PR needs rebase |
|
It's very hard to track, given GitHub's poor code review system, but I think there are comments from my original review which haven't yet been addressed. This is a big PR... |
|
Still working on the cleanup for rebase. But most of your comments should
be addressed. There are a couple of places where I didn't do the "if err =
..." as it would have yielded more complicated code. If you feel strongly
about it I can add them as well. :)
…On Mon, Dec 11, 2017 at 11:11 AM, Martin Taillefer ***@***.*** > wrote:
It's very hard to track, given GitHub's poor code review system, but I
think there are comments from my original review which haven't yet been
addressed.
This is a big PR...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1931 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AQR8I2MsayN30g87GvhLL6V9fdUwqaOIks5s_X5JgaJpZM4Qv4O5>
.
|
|
I know you commented that:
x.y, err = func()
if err != nil {
Would be more wordy as:
if x.y, err = func(); err != nil {
Isn't that shorter?
Note, this is = and not :=
On Mon, Dec 11, 2017 at 11:13 AM, Ozben Evren <notifications@github.com>
wrote:
… Still working on the cleanup for rebase. But most of your comments should
be addressed. There are a couple of places where I didn't do the "if err =
..." as it would have yielded more complicated code. If you feel strongly
about it I can add them as well. :)
On Mon, Dec 11, 2017 at 11:11 AM, Martin Taillefer <
***@***.***
> wrote:
> It's very hard to track, given GitHub's poor code review system, but I
> think there are comments from my original review which haven't yet been
> addressed.
>
> This is a big PR...
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1931 (comment)>, or
mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/
AQR8I2MsayN30g87GvhLL6V9fdUwqaOIks5s_X5JgaJpZM4Qv4O5>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1931 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AVucHaciDZfGYCLipY_zs-L5tXwsRPdYks5s_X7AgaJpZM4Qv4O5>
.
|
|
It is about having to hoist var declarations out. if the variable is being
declared within that line, it will only be available within the if-block,
which is useless. Either I have to flip the condition and rewrite the code,
or do explicit variable declaration. The trade-off seems wrong.
I am not a fan of the single-line if, but I don't feel strongly about it.
I'll rescan and make it uniform.
On Mon, Dec 11, 2017 at 11:17 AM, Martin Taillefer <notifications@github.com
… wrote:
I know you commented that:
x.y, err = func()
if err != nil {
Would be more wordy as:
if x.y, err = func(); err != nil {
Isn't that shorter?
Note, this is = and not :=
On Mon, Dec 11, 2017 at 11:13 AM, Ozben Evren ***@***.***>
wrote:
> Still working on the cleanup for rebase. But most of your comments should
> be addressed. There are a couple of places where I didn't do the "if err
=
> ..." as it would have yielded more complicated code. If you feel strongly
> about it I can add them as well. :)
>
> On Mon, Dec 11, 2017 at 11:11 AM, Martin Taillefer <
> ***@***.***
> > wrote:
>
> > It's very hard to track, given GitHub's poor code review system, but I
> > think there are comments from my original review which haven't yet been
> > addressed.
> >
> > This is a big PR...
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <#1931 (comment)>, or
> mute
> > the thread
> > <https://github.com/notifications/unsubscribe-auth/
> AQR8I2MsayN30g87GvhLL6V9fdUwqaOIks5s_X5JgaJpZM4Qv4O5>
> > .
>
> >
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1931 (comment)>, or
mute
> the thread
> <https://github.com/notifications/unsubscribe-
auth/AVucHaciDZfGYCLipY_zs-L5tXwsRPdYks5s_X7AgaJpZM4Qv4O5>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1931 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AQR8I7jHzaaBVZm7PDSKNF4FviRu2-ZFks5s_X_GgaJpZM4Qv4O5>
.
|
|
I specifically pointed out my comments are about lines with "=" in it, not
with ":=". That means no symbols are being introduced by such a line and so
a single-line "if" should be universally usable without changing any
variable scopes.
On Mon, Dec 11, 2017 at 11:24 AM, Ozben Evren <notifications@github.com>
wrote:
… It is about having to hoist var declarations out. if the variable is being
declared within that line, it will only be available within the if-block,
which is useless. Either I have to flip the condition and rewrite the code,
or do explicit variable declaration. The trade-off seems wrong.
I am not a fan of the single-line if, but I don't feel strongly about it.
I'll rescan and make it uniform.
On Mon, Dec 11, 2017 at 11:17 AM, Martin Taillefer <
***@***.***
> wrote:
> I know you commented that:
>
> x.y, err = func()
> if err != nil {
>
> Would be more wordy as:
>
> if x.y, err = func(); err != nil {
>
> Isn't that shorter?
>
> Note, this is = and not :=
>
> On Mon, Dec 11, 2017 at 11:13 AM, Ozben Evren ***@***.***>
> wrote:
>
> > Still working on the cleanup for rebase. But most of your comments
should
> > be addressed. There are a couple of places where I didn't do the "if
err
> =
> > ..." as it would have yielded more complicated code. If you feel
strongly
> > about it I can add them as well. :)
> >
> > On Mon, Dec 11, 2017 at 11:11 AM, Martin Taillefer <
> > ***@***.***
> > > wrote:
> >
> > > It's very hard to track, given GitHub's poor code review system, but
I
> > > think there are comments from my original review which haven't yet
been
> > > addressed.
> > >
> > > This is a big PR...
> > >
> > > —
> > > You are receiving this because you were mentioned.
> > > Reply to this email directly, view it on GitHub
> > > <#1931 (comment)>,
or
> > mute
> > > the thread
> > > <https://github.com/notifications/unsubscribe-auth/
> > AQR8I2MsayN30g87GvhLL6V9fdUwqaOIks5s_X5JgaJpZM4Qv4O5>
> > > .
> >
> > >
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <#1931 (comment)>, or
> mute
> > the thread
> > <https://github.com/notifications/unsubscribe-
> auth/AVucHaciDZfGYCLipY_zs-L5tXwsRPdYks5s_X7AgaJpZM4Qv4O5>
>
> > .
> >
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1931 (comment)>, or
mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/
AQR8I7jHzaaBVZm7PDSKNF4FviRu2-ZFks5s_X_GgaJpZM4Qv4O5>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1931 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AVucHUP4QrpoxzgLugabQSdhUVDatH3Dks5s_YFqgaJpZM4Qv4O5>
.
|
|
Any more comments on this? If not, I'd like to check this in. |
|
I had LGTM'd before, if there has not been any major changes since then, I
am good.
Thanks,
-Sunny
…On Tue, Dec 12, 2017 at 10:26 AM, Ozben Evren ***@***.***> wrote:
Any more comments on this? If not, I'd like to check this in.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1931 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AK6xE6I569KDB_QRcGTY4nBQ1l3TKSw2ks5s_sU6gaJpZM4Qv4O5>
.
--
*Thanks,*
*-SG*
|
|
/retest |
This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (istio#1938) a32a587 Add a check cache test for string map sub keys (istio#1931) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (istio#4190) 36809d80a fuzz: coverage profile generation instructions. (istio#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (istio#4075) 05c0d52d3 build: use clang-6.0. (istio#4168) 01f403ec4 thrift_proxy: introduce header transport (istio#4082) 564d256fb tcp: allow connection pool callers to store protocol state (istio#4131) 3e1d643b9 configs: match latest API changes (istio#4185) bc6a10c2f Fix wrong mock function name. (istio#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (istio#4182) 3d1325e89 Converting envoy configs to V2 (istio#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (istio#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (istio#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (istio#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (istio#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (istio#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (istio#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (istio#4157) 71152b710 ratelimit: Add ratelimit custom response headers (istio#4015) 306287418 ssl: make Ssl::Connection const everywhere (istio#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (istio#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (istio#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (istio#4130) 8c189a552 Make over provisioning factor configurable (istio#4003) 6c08fb43c Making test less flaky (istio#4149) 592775b7b fuzz: bare bones HCM fuzzer. (istio#4118) For istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (#1938) a32a587 Add a check cache test for string map sub keys (#1931) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (#4190) 36809d80a fuzz: coverage profile generation instructions. (#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (#4075) 05c0d52d3 build: use clang-6.0. (#4168) 01f403ec4 thrift_proxy: introduce header transport (#4082) 564d256fb tcp: allow connection pool callers to store protocol state (#4131) 3e1d643b9 configs: match latest API changes (#4185) bc6a10c2f Fix wrong mock function name. (#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (#4182) 3d1325e89 Converting envoy configs to V2 (#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (#4157) 71152b710 ratelimit: Add ratelimit custom response headers (#4015) 306287418 ssl: make Ssl::Connection const everywhere (#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (#4130) 8c189a552 Make over provisioning factor configurable (#4003) 6c08fb43c Making test less flaky (#4149) 592775b7b fuzz: bare bones HCM fuzzer. (#4118) For #7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
What this PR does / why we need it:
This PR introduces a new perf testing framework for Mixer. A substantial documentation can be found in the package documentation in setup.go.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
I am sending this out a bit early: it is going to conflict with some of the PRs that I know to be in-flight. I'll incorporate those changes when they land, before checking this PR in.
Also, this PR does not include an actual perf test (just a proof of concept one). I'll try to come up with a basic test separately, and submit it in a separate PR.
Release note: