685862 - implement throttling in AUS3

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), defect, P2)

Description

[[e-mail] (:bhearsum)]

Currently, AUS3 doesn't pay attention to throttling except for checking for non-zero. We need to replicate the AUS2 throttle logic in AUS3. That is:
- For forced update checks, ignore throttling
- For background checks:
-- When throttle=0, ignore the rule
-- When throttle=100, always consider the rule
-- When throttle is between 0 and 100, consider the rule that % age of the time. Eg, when throttle is 50 the rule should be considered half the time. In AUS2, this is implemented with rand().

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Add throttling support

This takes about 7 seconds to run on my laptop, and the 10% throttle test will fail in about once in 370 runs (3 sigma in a normal distribution, which we approximate pretty nicely by the time there's 10000 calls). I'd like to do better than that, somehow.

Forcing to override throttle=0 is my next job.

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Oh, known issue with that patch is that it throws a lot of debug logging at you if a test fails. At first glance the unittest module doesn't have a way of turning that behaviour off so that could be fun to deal with.

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 572417 [details] [diff] [review]
Add throttling support

Review of attachment 572417 [details] [diff] [review]:
-----------------------------------------------------------------

One thing you could do here to guarantee your results is mock out randint, too.

results = [100,100,100,100,50]
def se(*args, **kwargs):
  return results.pop()
with mock.patch('random.randint') as m:
  m.side_effect = se

(from http://www.voidspace.org.uk/python/mock/getting-started.html#raising-exceptions-with-mocks).

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 572417 [details] [diff] [review]
Add throttling support

Review of attachment 572417 [details] [diff] [review]:
-----------------------------------------------------------------

feedback+, regardless of whether you mock out randint or not.

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Forcing, throttling and cleanup

Highlights
* rename throttle -> backgroundRate everywhere, and also change update_type -> updateType to match the camel case used elsewhere in the Rules table. Do you prefer a_b_c ? Is backgroundRate OK ?
* redo backgroundRate implementation, and testing to not be stochastic
* implement forcing support, including passing force=1 on to bouncer. I couldn't find a nice way to unpack, append, and pack the query arguments so I did a fairly simple append

We'll need to do some ALTER statements on the Balrog dev instance for column renames in the rules.

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 580864 [details] [diff] [review]
Forcing, throttling and cleanup

Review of attachment 580864 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Nick Thomas [:nthomas] from comment #5)
> Created attachment 580864 [details] [diff] [review]
> Forcing, throttling and cleanup
> 
> Highlights
> * rename throttle -> backgroundRate everywhere, and also change update_type
> -> updateType to match the camel case used elsewhere in the Rules table. Do
> you prefer a_b_c ? Is backgroundRate OK ?

I think backgroundRate is a big improvement over throttle. ++ to that!

> * redo backgroundRate implementation, and testing to not be stochastic

Yay!

> * implement forcing support, including passing force=1 on to bouncer. I
> couldn't find a nice way to unpack, append, and pack the query arguments so
> I did a fairly simple append

urlparse.urlparse and urlparse.parse_qs might get you what you want, it's a bit of a weird library IME though.

> We'll need to do some ALTER statements on the Balrog dev instance for column
> renames in the rules.

I'm hoping we can get manage-db.py to do this. I can take a stab at that if you want.


This looks good to me overall, and the test coverage is great!

::: auslib/client/views/client.py
@@ +30,5 @@
>                        'osVersion': 'Darwin%2010.6.0',
>                        'distribution': 'default',
>                        'distVersion': 'default',
>                        'headerArchitecture': 'Intel',
> +                      'force': 1,

Can we use True/False for force instead of 1/0?

::: auslib/test/test_AUS.py
@@ +4,5 @@
> +from math import sqrt
> +
> +from auslib.AUS import AUS3
> +
> +def RandomAUSTest(AUS, backgroundRate, force):

I'm having trouble understanding this function, and how it returns early sometimes. How is that it that tested = 1 for testThrottling100, but tested = 100 for other tests? AFAICT AUS.rand.getRange() returns the same thing in all of the test cases.

Comment 7

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Ben Hearsum [:bhearsum] from comment #6)
> urlparse.urlparse and urlparse.parse_qs might get you what you want, it's a
> bit of a weird library IME though.

I'd noticed those, but by the time parse_qs returns a dict you end up rearranging the query params when reconstructing, plus there were some encoding issues too. I can take another look if the append I have is too fragile.
 
> > We'll need to do some ALTER statements on the Balrog dev instance for column
> > renames in the rules.
> 
> I'm hoping we can get manage-db.py to do this. I can take a stab at that if
> you want.
 
What sort of thing would this do ? Perhaps we need a separate table which stores some sort of database version ? I'd been thinking some sort global prefs might be useful (eg turn off all updates), which might fit well together.
 
> This looks good to me overall, and the test coverage is great!

Woo.
 
> Can we use True/False for force instead of 1/0?

I'll change that.
 
> > +def RandomAUSTest(AUS, backgroundRate, force):
> 
> I'm having trouble understanding this function, and how it returns early
> sometimes. How is that it that tested = 1 for testThrottling100, but tested
> = 100 for other tests? AFAICT AUS.rand.getRange() returns the same thing in
> all of the test cases.

Originally I wrote this so that randint was only mocked for a backgroundRate < 100, and no forcing (ie RandomAUSTest was in testThrottling50). Then I got paranoid and wanted to do some sort of check that if I make several requests with 100 or forced, then all of those requests found an update, and 0 really gets none, so I generalized the test. However, when you have rate of 100 or forcing you never pop off results and the while never ends, so I added the escape clause based on the length of results.

I quite like the end result, because it verifies that 100 or forcing never hits the randint, while backgroundRate < 100 does test all possible random values and gets the right answer. Open to changes though.

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #7)
> (In reply to Ben Hearsum [:bhearsum] from comment #6)
> > urlparse.urlparse and urlparse.parse_qs might get you what you want, it's a
> > bit of a weird library IME though.
> 
> I'd noticed those, but by the time parse_qs returns a dict you end up
> rearranging the query params when reconstructing, plus there were some
> encoding issues too. I can take another look if the append I have is too
> fragile.

I think what you've got is fine, just wanted to make sure you'd seen the urlparse library.

> > > We'll need to do some ALTER statements on the Balrog dev instance for column
> > > renames in the rules.
> > 
> > I'm hoping we can get manage-db.py to do this. I can take a stab at that if
> > you want.
>  
> What sort of thing would this do ? Perhaps we need a separate table which
> stores some sort of database version ? I'd been thinking some sort global
> prefs might be useful (eg turn off all updates), which might fit well
> together.

I was just thinking that manage-db.py should probably be the thing doing schema changes and other "upgrades" to the database - rather than someone doing it by hand. I hadn't thought in detail about it much, but having a database version seems like it would make that task a lot simpler.

> > This 
> > > +def RandomAUSTest(AUS, backgroundRate, force):
> > 
> > I'm having trouble understanding this function, and how it returns early
> > sometimes. How is that it that tested = 1 for testThrottling100, but tested
> > = 100 for other tests? AFAICT AUS.rand.getRange() returns the same thing in
> > all of the test cases.
> 
> Originally I wrote this so that randint was only mocked for a backgroundRate
> < 100, and no forcing (ie RandomAUSTest was in testThrottling50). Then I got
> paranoid and wanted to do some sort of check that if I make several requests
> with 100 or forced, then all of those requests found an update, and 0 really
> gets none, so I generalized the test. However, when you have rate of 100 or
> forcing you never pop off results and the while never ends, so I added the
> escape clause based on the length of results.
> 
> I quite like the end result, because it verifies that 100 or forcing never
> hits the randint, while backgroundRate < 100 does test all possible random
> values and gets the right answer. Open to changes though.

Ah, I understand now. My brain was thinking that the side effect function got called with every iteration. Now that I'm set straight it looks good to me!

So, r+ with the True/False change but we need to figure out the upgrade story still.

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Fix AUS-server.py logging

https://github.com/nthomas-mozilla/balrog/tree/throttling has the True/False change for force.

This sets the options on the logger we instantiated before the other modules are loaded and have a chance to set a root handler.

Comment 10

[[e-mail] (:bhearsum)]

Comment on attachment 581862 [details] [diff] [review]
Fix AUS-server.py logging

Ah! Good catch!

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 581862 [details] [diff] [review]
Fix AUS-server.py logging

https://github.com/mozilla/balrog/commit/eea939ec5b3b3e884568c5bbfae8811a21c0d3ce

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: As committed

https://github.com/mozilla/balrog/commit/720ea0fc34fb96a19b47814e8015af81fed161d2

Need a bit of a merge to land, and I moved the column renames out to bug 733274.

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

Followup for the new test file which got missed:
 https://github.com/mozilla/balrog/commit/f3a533dc7be58ec04cbd9769246ca8f5933e7372

Comment 14

[[e-mail] (:bhearsum)]

mass component change