Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Advanced Circuit Breaker #1153

Merged
merged 5 commits into from
Apr 24, 2023
Merged

Conversation

martintmk
Copy link
Contributor

@martintmk martintmk commented Apr 24, 2023

The issue or feature being addressed

Closes #1098

Details on the issue fix or feature implementation

In this PR I am implementing AdvancedCircuitBehavior which finalizes the Circuit Breaker for V8. No public API changes.
I have copied and cleaned-up the RollingHealthMetrics and SingleHealthMetrics from Polly V7 to preserve the behavior.

Additional changes:

  • Added benchmarks for Circuit Breaker

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

@martintmk martintmk added the v8 Issues related to the new version 8 of the Polly library. label Apr 24, 2023
@martintmk martintmk added this to the v8.0.0 milestone Apr 24, 2023
@martintmk martintmk self-assigned this Apr 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2023

Codecov Report

Merging #1153 (bc0554c) into main (c7c8dbd) will increase coverage by 0.25%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1153      +/-   ##
==========================================
+ Coverage   80.67%   80.92%   +0.25%     
==========================================
  Files         233      237       +4     
  Lines        5427     5499      +72     
  Branches      922      930       +8     
==========================================
+ Hits         4378     4450      +72     
  Misses        844      844              
  Partials      205      205              
Flag Coverage Δ
linux 80.92% <100.00%> (+0.25%) ⬆️
macos 80.92% <100.00%> (+0.25%) ⬆️
windows 80.92% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...itBreaker/AdvancedCircuitBreakerStrategyOptions.cs 100.00% <ø> (ø)
...ircuitBreaker/BaseCircuitBreakerStrategyOptions.cs 100.00% <100.00%> (ø)
...rcuitBreakerResilienceStrategyBuilderExtensions.cs 100.00% <100.00%> (ø)
...rcuitBreaker/Controller/AdvancedCircuitBehavior.cs 100.00% <100.00%> (ø)
src/Polly.Core/CircuitBreaker/Health/HealthInfo.cs 100.00% <100.00%> (ø)
.../Polly.Core/CircuitBreaker/Health/HealthMetrics.cs 100.00% <100.00%> (ø)
...Core/CircuitBreaker/Health/RollingHealthMetrics.cs 100.00% <100.00%> (ø)
....Core/CircuitBreaker/Health/SingleHealthMetrics.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment on lines +54 to +55
| ExecuteCircuitBreaker_V7 | 198.4 ns | 2.78 ns | 3.99 ns | 1.00 | 0.00 | 0.0629 | 528 B | 1.00 |
| ExecuteCircuitBreaker_V8 | 297.9 ns | 2.63 ns | 3.77 ns | 1.50 | 0.04 | 0.0038 | 32 B | 0.06 |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that it's slower.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will give this another shot later to see if we can bring it down. Allocations are greatly reduced though, that for us makes bigger impact.

_windows.Enqueue(_currentWindow);
}

while (now - _windows.Peek().StartedAt >= _samplingDuration)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Peek() guaranteed to return a non-null value here? Would it be possible for this and Reset() to race and cause _currentWindow to become null while the method is running?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's executed under a lock. Will add a note to the docs about it.

@martintmk martintmk merged commit a1ef1bb into main Apr 24, 2023
@martintmk martintmk deleted the mtomka/advancedcircuitbehavior branch April 24, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 Issues related to the new version 8 of the Polly library.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Introduce Circuit Breaker Strategy
3 participants