This is an OK starting point. But this is one of those things that doesn't scale very well.

e.g. imagine we get 100x the number of requests, because of generic growth. Then, even if the transient error rate is 0.0001%, eventually you'd hit this static threshold.

So again, this is fine for now. But just keep in the back of your mind that we'll want to make this a bit more robust in the future. Perhaps by checking a "percentage of all requests within that time window", etc.

Minor-nit: This is totally OK. But I'm guessing that we have some idea of how many records we want to store. So it would be a tiny bit more efficient to pre-allocate the space. e.g. make(map[string][]*record{}, 1024).

Note that you only really benefit from that if the map will grow considerably (e.g. contain thousands of elements. If we only expect ~dozens or ~hundreds of elements, then this would fall into the 🤷 not worth it camp.)

⚠️ this is really bad from an efficiency perspective.

• You are creating a new slice with a single element. ([]*record{r})
• You are then appending records to the end of it. (Which will need to resize the newly created slice, and copy over all the elements.)

Instead, you should just write this as:

records = append(records, r)

This will keep the data in the records slice as-is, and only copy the new element over. If the underlying slice didn't have enough space, only then would it "copy all the elements into a new slice".

Does that all make sense?

Addressed when implementing suggestion from https://github.com/sourcegraph/sourcegraph/pull/64133#discussion_r1695520961 by creating a slice with a predefined length and assigning new values by index instead of appending. Is this approach sustainable?

https://github.com/sourcegraph/sourcegraph/blob/6080a047e634cbf76d755bfd44974795423105a1/cmd/cody-gateway/internal/httpapi/completions/modelsloadtracker.go#L82-L96

by creating a slice with a predefined length and assigning new values by index instead of appending. Is this approach sustainable?

Yup, SGTM. I think it would be arguably, slightly cleaner to use a fixed-width array. (e.g. the type of records being [20]record.) But that's more of a matter of style.

The key thing is that once we introduce the modelCircuitBreaker, that we don't do a bunch of unnecessary copying/resizing of the slice. (Which we avoid by using that headIndex and just overwriting previous failures.)

Consider moving the mutex down into the modelCircuitBreaker. That way we can record failures for two different models simultaneously. It's just a way of further reducing the possibility of contention on the lock.

With the updated updated implementation (https://github.com/sourcegraph/sourcegraph/pull/64133#issuecomment-2263187863) we have two mutexes: one for modelsLoadTracker (to lock on modelCircuitBreaker creation) and one on modelCircuitBreaker to lock on adding record and calculating the failure ratio. Does this approach sound reasonable to you?

Nit: What do you think about maxFailures or even maxAllowedFailures as a name? failureThreshold is definitely OK. But max-* might seem like a better fit? WDYT?

With the updated implementation (https://github.com/sourcegraph/sourcegraph/pull/64133#issuecomment-2263187863) I renamed this to failureRatio, WDYT?

Nit: having reason as a string is helpful for the developer, but probably is just more overhead than is useful. Not that we need to worry about being ultra-efficient here, but if we just had this as statusCode int and just kept track of whatever the "non-2xx" HTTP response status code was, that'd be sufficient IMHO. But this certainly will make it easier to diagnose.

Changed to use HTTP status codes as reasons in https://github.com/sourcegraph/sourcegraph/pull/64133/commits/c1c1db4cd9865c06998fca286634696a423723e7.

just kept track of whatever the "non-2xx" HTTP response status code was

We consider the a model unavailable if requests to it fail because of a context.DeadlineExceeded error or result in responses with 429 status codes. We ignore other non-2xx status codes. Do you think we should track all non-2xx responses?

WDYT, @chrsmith?

I've left a comment elsewhere on the PR. I could be convinced either way how to approach this.

⚠️ Is this the behavior that we want? I guess I misunderstood the exact way you were thinking about enforcing the circuit breaker. But I think we should expect some non-zero amount of "random, transient bullshit" from the upstream LLM provider. (e.g. a network cable gets cut, or some random hardware outage on their side.)

So having Cody Gateway consider a model unavailable with as few as a single error seems too sensitive. I thought we would instead require failureThreshold errors within timeout seconds. Does that seem like a better approach, or am I missing something?

Is this the behavior that we want?

Same as https://github.com/sourcegraph/sourcegraph/pull/64133#discussion_r1698293305.
Thanks for taking another look at the PR, I'm happy that we have this discussion on the PR, but not as a post-mortem 😅

we would instead require failureThreshold errors within timeout seconds

I don't think it'll work. Let's assume the failureThreshold is 10.
If within the timeout period we get 10,000 requests and only record failureThreshold number of errors, the model should still be considered available (as only 0.001% of requests indicate an issue). However, if within the timeout period we get 12 requests and 10 of them result in a 429 error, the model should be considered temporarily unavailable.

So, maybe we should implement the "percentage of all requests within the time window" approach you suggested in https://github.com/sourcegraph/sourcegraph/pull/64133#discussion_r1695504899?

implement the "percentage of all requests within the time window" approach

I’ve started exploring the implementation of X percent of failing requests out of the total requests within a Y-minute timeframe. I checked the Cody Gateway logs and noticed that it receives over 3k requests per minute to /v1/completions/fireworks. Over 5 minutes (the current timeout in the PR), this would obviously exceed 15k requests. I’m concerned that storing this many records in memory could be excessive, and iterating through the slice to determine model availability for each request might be even more critical performance wise.

Taking a lock every time we need to load a model circuit breaker isn't great. As it essentially means we can never do so in parallel.

If we are using a sync.RWMutext, consider using only RLock() or read-only lock. That will be the 99% case where we only want to get an existing value. (And there can be any number of concurrent reads.)

And if and only if the model isn't found, then we release the read-lock, and wait for a write-lock (r.Lock()) instead.

HOWEVER... rather than manually do all of this, we should probably just use the built in sync.Map type. Which exists to provide a safe-for-concurrency Map type, which is exactly what we are trying to do here.

https://pkg.go.dev/sync#Map

And then we'd just always call the Load method, and if the key wasn't found then call LoadOrStore. (We don't want to always call LoadOrStore, because that would require us to allocate the memory for adding a new ModelCircuitBreaker every type. When that's the var less common case.)

Anyways, in case you are curious, if you look at the implementation is kinda sorta what I was suggesting about only taking a read-lock, and then if the key isn't present, then taking a write-lock to add it. (Read: under the hood, it still uses a RWMutex. It's just that the sync.Map does some other things to make the common case faster.)
https://cs.opensource.google/go/go/+/refs/tags/go1.22.5:src/sync/map.go;l=203

I've left a comment elsewhere on the PR. I could be convinced either way how to approach this.

I think we need to be very careful here. Consider:

• Having the if x == a || x == b || x == c { can get tedious. Using a switch statement can in some cases make this easier to read.
• Since we are using the status code of '0' to denote that there was some failure even getting a response from upstream, we might want to consider those as failures, too?
• Including the upstream LLM provider giving us a 500 seems like a good failure to be aware of as well.

switch r.statusCode {
    case http.StatusTooManyRequests, http.StatusGatewayTimeout, 0 /* no response obj */, http.StatusInternalServerError:
        failedCount++
}

I think this is the right approach, instead of say only considering 2xx requests as "non-failures". Because a user could have a misconfigured client, or just be sending bad requests resulting in a 4xx. And we shouldn't let errors on the client-side "poison" a model for all of Cody Gateway.

So being explicit and only considering a handful of HTTP responses as "failures that would count towards temporarily disabling the model" seems like a good choice.

Refactored to use a switch statement. I didn’t include the 0 case, as it also covers request errors like context.Canceled, which are unlikely to be upstream provider errors.