[ingester] Optionally delay circuit breaker activation until after traffic starts

[pr00se]

What this PR does

Change the implementation of the --initial-delay option for ingester circuit breakers as follows:

• Start the activation delay with the first call to tryAcquirePermit() rather than the call to activate(). This lets us ignore the first initial_delay of traffic, regardless of how long after activation it arrives.
• If activate() is called during the activation delay, don't schedule another delayed activation
• If deactivate() is called during the activation delay, cancel any pending activation

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

• Tests updated.
• Documentation added.
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• about-versioning.md updated with experimental features.

[tacole02]

Thanks for updating these docs! I massaged the wording a little bit.

[tacole02]

Suggested change

	# (experimental) How long after an initial request an activated circuit
	# breaker should wait before becoming effectively active. During this time
	# neither failures nor successes will not be counted.
	# (experimental) Duration, in seconds, after an initial request an activated circuit
	# breaker should wait before becoming effectively active. During this time,
	# neither failures nor successes are counted.

[pr00se]

addressed in 24a3b0d

[tacole02]

Same suggestion as above here.

[pr00se]

addressed in 24a3b0d

[duricanikolic]

Left a couple of nits

[duricanikolic]

I guess here we rely on the fact that within time.AfterFunc() we actually set cb.activationTimer to nil.
Are we sure it can never happen that we reach this line while time.AfterFunc() is executing?

I personally prefer usage of time.NewTimer() because then time.Stop()'s result (true or false) clearly explains what happened. Here false means that the timer expired, and the function passed to time.AfterFun() was started in a separate go routine, but doesn't say anything about whether the function was completed or not.

Anyway, if we opt for this approach, since it is crucial to execute cb.activationTimer = nil, I would put a comment on both lines that execute it (251 and 284) to highlight that those lines must not be deleted or moved from those places, because that would break the logic.

[pr00se]

If we are here we are holding stateMtx, which is also taken at the start of the execution of the AfterFunc(), so if we are here while that function is executing, it is waiting on the mutex. once it gets the mutex it will see that the state is not circuitBreakerActivating and exit.

i believe these are the possible situations:

1. the timer hasn't yet expired, and we successfully Stop() it here -- the return value would be true
2. the timer expired and the launched goroutine completed some time before deactivate() was called -- in this case the timer will be nil (because the timer function cleared it) and we won't call Stop() at all
3. the timer expired and the launched goroutine is currently executing when deactivate() is called -- in this case deactivate() will block because the timer goroutine is holding stateMtx, and then we'll be in the same state as (2)
4. the timer expired and launched the goroutine while deactivate() was executing -- in this case we would call Stop() and it would return false; the launched goroutine would be waiting on the stateMtx, and after deactivate() completes, would get it, and exit because the state is no longer circuitBreakerActivating

i don't believe the lines that set the timer to nil are strictly necessary. if we removed then, then we would just additionally call Stop() on an expired timer in cases (2) and (3) above.

---

i opted not to use time.NewTimer()to avoid launching a separate goroutine to poll the timer channel (and have to cancel when we Stop() the timer, since otherwise i think it would block forever)

[duricanikolic]

Do you think it is feasible to add a test that would simulate what happens if we:

• at timestamp ts: schedule a cb activation with delay 3*d
• at timestamp ts + d: deactivate the cb
• at timestamp ts + 2d: schedule another cb activation with delay 3*d
• ensure that the cb is not active before timestamp ts + 5d?

[pr00se]

added in c67e4e3

[duricanikolic]

LGTM
Thank you @pr00se

[grafanabot]

The backport to r316 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-9842-to-r316 origin/r316
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x c56a74829dd8135a4b3a93ebf5f5222c4f1ac117
# Push it to GitHub
git push --set-upstream origin backport-9842-to-r316
git switch main
# Remove the local backport branch
git branch -D backport-9842-to-r316

Then, create a pull request where the base branch is r316 and the compare/head branch is backport-9842-to-r316.