fix: correct EWMA decay formula and reject sample count of 0#27
Open
costela wants to merge 1 commit into
Open
Conversation
The smoothing factor was computed as 2/(N/2+1), which is the span formula with N halved. For sampleCount == 1 this yields decay == 1.333 - a smoothing factor greater than 1, giving the previous failure rate a negative weight so the rate can go below 0 or above 1. It also contradicted the documented "a value of 1 causes a single sample to be considered": that single-sample behavior (decay == 1) actually only occurred at sampleCount == 2. Use the canonical span-based factor 2/(N+1) (the Wikipedia exponential- smoothing link in the code): it equals exactly 1 at N == 1 (matching the docs) and stays in (0,1] for every N >= 1. Only N == 0 remains out of range (decay == 2), so apply now rejects it. The zero-value breaker (decay == 0) stays allowed and never opens, as documented. This changes convergence speed for existing sample counts, so it is a behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the EWMA breaker’s smoothing/decay factor to use the canonical span-based formula and ensures invalid configuration (sampleCount == 0) is rejected during circuit construction, preventing out-of-range EWMA weights.
Changes:
- Update EWMA decay formula to
2/(N+1)soN==1yields decay1and allN>=1stay within(0,1]. - Add validation in
EWMABreaker.applyto reject configurations that producedecay > 1(i.e.,sampleCount == 0). - Add tests covering the
sampleCount == 1decay behavior andsampleCount == 0rejection viaNewCircuit.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| breaker.go | Corrects EWMA decay computation and adds sampleCount==0 rejection via apply() validation. |
| breaker_test.go | Adds regression tests for decay correctness at sampleCount==1 and rejection at sampleCount==0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Corrects the EWMA smoothing factor and rejects
sampleCount == 0.Why
The decay was
2/(N/2+1)— the span formula withNhalved:sampleCount == 1→ decay 1.333 (> 1), giving the previous rate a negative weight, so the failure rate can go<0or>1. Mathematically broken.sampleCount == 2.Anyone who read the doc and passed
sampleCount=1is running a breaker with a broken update rule.How
2/(N+1): exactly1atN==1(matches the docs), and in(0,1]for allN≥1.N==0is left out of range (decay 2) →applyrejects it with a clear error.Notes
feat!type-parameter change.🤖 Generated with Claude Code