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

Calculated break duration for Circuit breaker #1776

Merged
merged 40 commits into from
Nov 8, 2023

Conversation

atawLee
Copy link
Contributor

@atawLee atawLee commented Nov 6, 2023

The issue or feature being addressed

#653

Details on the issue fix or feature implementation

  • Added a FailureCount get property to CircuitBehavior to retrieve the count of failures.
  • Introduced a private readonly field _breakDurationGenerator of type Func<BreakDurationGeneratorArguments, ValueTask<TimeSpan>>? to CircuitStateController for dynamic break duration generation.
  • Enhanced the constructor of CircuitStateController with an overload to accommodate _breakDurationGenerator.
  • Incorporated FailureCount into HealthInfo.
  • Updated OpenCircuitFor_NeedsLock to assign the _blockedUntil value based on both the presence of breakDuration and the number of failures.

This new Pull Request is a continuation of the work on the GeneratorDuration branch. It includes all the changes previously submitted and any additional fixes required to align with the main branch.
#1715

I am currently working on improving the Test Coverage to ensure the robustness of the new features. This work is still in progress, and I will update this PR with the relevant changes as soon as they are ready.

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

atawLee and others added 30 commits October 22, 2023 15:19
- CircuitStateControllerTest
- RollingHealthMetrixTest
apply code convention feedback

Co-authored-by: Martin Costello <martin@martincostello.com>
Add CircuitBreaker BreakDurationGenerator

Document
Update Usage
Update Anti-pattern
Add CircuitBreaker BreakDurationGenerator

Document
Update Usage
Update Anti-pattern
- CircuitStateControllerTest
- RollingHealthMetrixTest
apply code convention feedback

Co-authored-by: Martin Costello <martin@martincostello.com>
Update conventions and comments

Co-authored-by: martintmk <103487740+martintmk@users.noreply.github.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Update conventions and comments

Co-authored-by: Martin Costello <martin@martincostello.com>
HealthInfo - Remove Default Parameter
Argument - Missing XML documentation for the constructor and properties, ResilienceContext
Modify code conventions

Co-authored-by: martintmk <103487740+martintmk@users.noreply.github.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
atawLee and others added 2 commits November 8, 2023 05:34
modify convention

Co-authored-by: Martin Costello <martin@martincostello.com>
Upon further review, it has been confirmed that the warning in question is valid, and after checking the git Action, it has become apparent that this part is causing the issue. Therefore, I will reapply the disable pragma for this section.
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (41fd38c) 84.53% compared to head (01b35fc) 84.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1776      +/-   ##
==========================================
+ Coverage   84.53%   84.56%   +0.03%     
==========================================
  Files         307      308       +1     
  Lines        6777     6791      +14     
  Branches     1043     1044       +1     
==========================================
+ Hits         5729     5743      +14     
  Misses        839      839              
  Partials      209      209              
Flag Coverage Δ
linux 84.56% <100.00%> (+0.03%) ⬆️
macos 84.56% <100.00%> (+0.03%) ⬆️
windows 84.56% <100.00%> (+0.03%) ⬆️

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

Files Coverage Δ
.../CircuitBreaker/BreakDurationGeneratorArguments.cs 100.00% <100.00%> (ø)
...itBreaker/CircuitBreakerStrategyOptions.TResult.cs 100.00% <100.00%> (ø)
...rcuitBreaker/Controller/AdvancedCircuitBehavior.cs 100.00% <100.00%> (ø)
...ircuitBreaker/Controller/CircuitStateController.cs 100.00% <100.00%> (ø)
src/Polly.Core/CircuitBreaker/Health/HealthInfo.cs 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@martintmk martintmk left a comment

Choose a reason for hiding this comment

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

Looks good. Great work and thanks for contributing :)

@martincostello martincostello added this to the v8.2.0 milestone Nov 8, 2023
@martincostello martincostello merged commit 2606c6a into App-vNext:main Nov 8, 2023
18 checks passed
@peter-csala
Copy link
Contributor

@atawLee Is the related documentation purposefully left untouched (until v8.2.0 is not released)?

@martincostello
Copy link
Member

I think that's just an oversight (it certainly is that I didn't notice we hadn't changed it).

@atawLee
Copy link
Contributor Author

atawLee commented Nov 10, 2023

@peter-csala it seems there might have been an oversight regarding the documentation update. Could you please guide me on which specific documents need to be revised?

@peter-csala
Copy link
Contributor

peter-csala commented Nov 10, 2023

@atawLee

circuit-breaker.md + CircuitBreaker.cs

  • Link
  • Under the Usage section
    • Please add a new sample option (between optionsComplex and optionsShouldHandle) which demonstrates the usage of the BreakDurationGenerator
    • In order to do that extend the Usage method of the CircuitBreaker class
  • Under the Defaults section
    • Please add a new row (between BreakDuration and OnClosed) which details the BreakDurationGenerator property
  • Under the Diagrams section
    • You can add here a new subsection with a sequence diagram to demonstrate how CB dynamically generates break duration timespans
    • If you think that this use case is really similar to one of the existing sequence diagrams then simple ignore this
  • Under the Anti-patterns section
    • Delete the Using different duration for breaks subsection
    • Delete the AntiPattern_SleepDurationGenerator method from the CircuitBreaker class

Tips

  • Use the dotnet mdsnippets command to refresh the code snippets inside the markdown files
    • This will update the main readme.md file as well
  • Use the dotnet docfx docs/docfx.json --serve command to check out locally the generated docfx pages under the http://localhost:8080/

@martincostello
Copy link
Member

@atawLee Will you be able to raise a PR to make the documentation updates for your change?

@atawLee
Copy link
Contributor Author

atawLee commented Nov 15, 2023

@martincostello Hi there, I'm currently working on the documentation updates for my change. I expect to be able to raise a Pull Request by the end of this week.

@atawLee atawLee mentioned this pull request Nov 17, 2023
4 tasks
@adminnz
Copy link

adminnz commented Dec 8, 2023

There is an issue with this. When you call AddCircuitBreaker(options), having specified BreakDurationGenerator. It does not pass the generate to the CircuitStateController. So the resulting circuitbreaker will use the default 5 seconds fixed duration.

@martincostello
Copy link
Member

@adminnz Thanks for reporting this. Would you mind creating a new issue with more details about your use case, then we can look at a fix.

@martincostello
Copy link
Member

@adminnz I've copied your comment to a new issue #1850 - could you please add a bit more detail there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants