feat(matching): Provide DynamicConfig option to override RPS of a specific TaskList

[joannalauu]

Provide new DynamicConfig option matching.overrideTaskListRps to override the RPS of a specific TaskList in TaskListLimiter

Why?

• During incident mitigation we may want to rapidly increase/decrease the RPS value of a TaskList. Providing a DynamicConfig to manipulate RPS can reduce remediation time.

Part of: #7296

How did you test it?

• Unit tests

Potential risks

• The system cannot adapt dynamically to changing load while the override is active, so it may impact system reliability if not carefully managed

Release notes

Documentation Changes

[natemort]

Awesome work! I'm going to have one other colleague review it, but this is great. The tests are solid

[natemort]

This is equivalent to:

overrideRps == 0 || overrideRps != current

which is a bit simpler to read.

[fimanishi]

I know we don't expect, but shouldn't we protect against negative values? Instead of using == 0 maybe using <= 0

[natemort]

Treating negative values the same as zero seems reasonable, yeah. Good call.

[fimanishi]

We should probably use overrideRPS > 0

[davidporter-id-au]

to more or less echo what Felipe and Nate were saying, may I suggest flattening the choice of using the passed in RPS or the dynconfig override RPS?

One way to do this is to override the rps float:

func (l *taskListLimiter) ReportLimit(rps float64) {
    // at the start, just figure out what the RPS value should be
	overrideRPS := l.overrideRPS()
    if overrideRPS > 0 {
        rps = overrideRPS
     }

     // rest of the function remains the same I think?

The reason for doing so is that if/else branching multiplies the codepaths that each execution could take, making it harder for programmers - looking at the code- to know which one it took, or which one is even valid.

I say this with some acknowledgement upfront that I've not looked at this code for a while and I might be missing some important context, so feel free to disagree.

[davidporter-id-au]

ah... I did a little more reading of the code here and I noticed a possible problem: The setting of the limit here will only take effect if the maxDispatchPersecond is already set by the customer, implying that this limit cannot be applied if they've added that config already.

May I suggest, a second thing, which, I know is annoying, but which would address this:

If you pull this override check up one level, into taskListManagerImpl, perhaps the getTask() method, there you can check the override pass it in without adding too much complexity to the RPS values.

eg:

    rps := 0
	if maxDispatchPerSecond != nil {
		rps = maxDispatchPerSecond
	}
   rpsOverride := l.overrideRPS()
   if rpsOverride > 0 {
       rps = rpsOverride
   }
  if tasklistRPS > 0 {
     c.limiter.ReportLimit(rps)  // unrelated to your change, the name 'Report' here is really confusing 😿. I guess we should change it at some point. 
   }

[natemort]

Thanks again for your work on this, I'll approve this without the changes to task_list_limiter.go.

[natemort]

Your change here is the most correct way to implement this, but I think for the sake of simplicity we don't need to add the override check here. The override applying to the incoming values is good enough and defines a very clear contract of how the components work.

If the override is smaller than the current RPS then it'll immediately take, and if it's larger then it'll take after the TTL.

Having a delay of up to a minute (the default TTL) isn't a big deal, the time for a user to react and make the config change is significantly higher.

[natemort]

Since we'll be using it only within task_list_manager.go we no longer need to add this here. I think this file ultimately will be unchanged, apologies for the iteration. I think David's proposal (applying it to incoming values) for how to structure it is better than mine.

[natemort]

Again, nice work on the tests. This code is not easy to test.

[natemort]

Nit: You could also consider testing the default case (no maxDispatchPerSecond, no overrideRps) and the override only case (no maxDispatchPerSecond, overrideRps > 0).