Fix delay behavior

[constanca-m]

Issue is #619.

[constanca-m]

@vigneshshanmugam Can I ask your opinion on this logic before I do the same for the local rate limiter?

This bug was added to GA goals: #619

This implementation has some things that worry me, but I don't if it makes sense to be worried over this:

1. What if we receive tons of requests? The delay will be increasing for each request, and then we will have many processes on hold. I think it makes sense to maybe set the delay to a fixed time, and after that time, each request starts getting rejected, regardless of the strategy. WDYT?
2. What if we receive requests like this:
   • We get a request that takes 10 tokens
   • We then get a request that takes 4 tokens
   • Both are delayed. Should the request of 4 tokens take priority, thus less delay for it, or should the 10 tokens have priority since it arrived first?

[vigneshshanmugam]

I dont think you need to do this, The issue here is that we are allowing all the clients to retry and send data simultanously once we past the reset time which is wrong. Its assuming the reset time guarentees the availability of tokens - requests/bytes based on the strategy.

The ideal way we could fix is basically looping through and retry after the delay, re checking the rate limits at each time. This respects all the configurations like strategy/algorithm along with keeping the race condition in check.

Rough sudo code would be

for {
  // check rate limit
 resp = getCurrentLimits()
 if resp.IsUnderLimit {
  // not limited
   return nil
 }
 
// we shouldn't use createAt here its flawed as its based on when the request is made and doesnt take retries in to account. 
  delay := time.Duration(resp.GetResetTime()-time.Now().UnixMilli()) * time.Millisecond
  // same code as before



} 

Hope this helps. Let me know if you want more details.

[constanca-m]

Thanks! This helped. I have updated the code, it should be working correctly now

[vigneshshanmugam]

this feels incorrect, we are using the old response, we need to get the new response after retry.

[constanca-m]

It's the new response, it changed on makeRateLimitRequest() call two lines above:

opentelemetry-collector-components/processor/ratelimitprocessor/gubernator.go

Line 154 in 26ae328

resp = responses[0]

[vigneshshanmugam]

Ack, it was quite confusing with the diff. Can we change the function to return resp, err so its easier to reason this? Thanks.

[constanca-m]

Sure thing, I have changed the code