GCE e2e build/test passed for commit e1fc9d8d757fd4e71c18cb8a90de45cdb3309de3.

• Build Log
• Test Artifacts
• Internal Jenkins Results

LGTM

GCE e2e build/test passed for commit f265d5c.

• Build Log
• Test Artifacts
• Internal Jenkins Results

cc @davidopp @lavalamp for second LGTM, etcetc

Hm-- are people setting rate limits high enough that this is a problem in practice?

And in which binaries is it a problem?

Rates higher than 10/second have noticeable CPU usage (several components set QPS of 20 by default), and when multiple clients are created, it causes issues (since the spawned tickers live forever, since clients don't surface a way to call Stop()).

OK. LGTM

@davidopp is sick; @quinton-hoole for ok-to-merge

Taking a look...

Are we OK taking on a whole new dependency by all our components on juju just before v1.0, and just for a more efficient rate limiter library? How complex would it be to just improve our current rate-limiter implementation instead?

cc @thockin @bgrant0607 for their opinion

I tried tweaking the existing implementation first, but the ticker at the heart of it was the root cause of the CPU usage at rest... I could see deferring this post-1.0

(though the CPU impact can become significant with higher QPS... we were seeing 20%ish usage in a vagrant env at rest with no data loaded)

That's just horrible, and almost definintely needs to be fixed pre-v1.0. The only question in my mind is whether we implement our own efficient rate limiter (which doesn't seem tremendously difficult), or take on the big juju dependency instead. I'm leaning towards the former.

I was on the fence about this, too, the dependency seems overkill. But it is expedient, assuming it works.

I didn't enjoy the dep, either, FWIW, but it's tiny.

OK, lets merge this PR, and open a separate issue to remove the dependency.

LGTM