#2080 Add new V8 params to fine-tune Polly's circuit-breaker behavior #2081

RaynaldM · 2024-05-29T14:05:53Z

Closes #2080

FailureRatio and SamplingDuration parameters of Polly V8 circuit-breaker #2080

This PR adds 2 new parameters in QoSOptions to fine-tune circuit-breaker behavior 👉

FailureRatio: The failure-success ratio that will cause the circuit to break/open.
SamplingDuration: The time period over which the failure-success ratio is calculated (in seconds).

raman-m · 2024-05-29T16:14:15Z

Great! Which version are you planning to release it in?

Please note that the PR depends on #2073, which needs to be merged before this PR. Therefore, this feature branch should be rebased after the merging of #2073, and ideally, after #2079 is merged as well.

It appears that it won't be included in version 23.3, but rather in version 24.0, which is the next release following v23.3.
Is this delivery acceptable to you?

RaynaldM · 2024-05-30T09:19:12Z

Great! Which version are you planning to release it in?

Please note that the PR depends on #2073, which needs to be merged before this PR. Therefore, this feature branch should be rebased after the merging of #2073, and ideally, after #2079 is merged as well.

It appears that it won't be included in version 23.3, but rather in version 24.0, which is the next release following v23.3. Is this delivery acceptable to you?

Yes, @raman-m, you can add this when you want

raman-m · 2024-06-11T16:47:56Z

@RaynaldM
Please resolve conflicts!
PR has been added to v24.0 milestone, current release

raman-m · 2024-06-13T10:18:12Z

@RaynaldM Conflicts!

raman-m · 2024-08-12T12:48:16Z

Let's assign this to the Sep'24 milestone since the Annual 2023 milestone is already quite overloaded, and we need to reduce the scope of work for 2023.

…ircuit-breaker behavior

add an isValid method on QoSOptions

raman-m

The absence of tests for the new properties is a significant concern for approval.
I recommend further work on the PR.

For Your Information: The feature branch has been successfully rebased onto the develop branch, as of November 1st, just today

raman-m · 2024-11-01T18:03:33Z

docs/features/qualityofservice.rst

@@ -35,13 +35,18 @@ Then add the following section to a Route configuration:
  "QoSOptions": {
    "ExceptionsAllowedBeforeBreaking": 3,
    "DurationOfBreak": 1000,
-    "TimeoutValue": 5000
+    "TimeoutValue": 5000,
+	"FailureRatio": .8 // .8 = 80% of failed, this is default value


No comma❗

Suggested change

"FailureRatio": .8 // .8 = 80% of failed, this is default value

"FailureRatio": .8, // .8 = 80% of failed, this is default value

raman-m · 2024-11-01T18:05:22Z

docs/features/qualityofservice.rst

-    "TimeoutValue": 5000
+    "TimeoutValue": 5000,
+	"FailureRatio": .8 // .8 = 80% of failed, this is default value
+	"SamplingDuration": 10000 // The time period over which the failure-success ratio is calculated (in milliseconds), default is 10000 (10s)


Better to add new properties docs below, after line 45.

You are wrong regarding default value! According to the docs for SamplingDuration, the default value is 30 seconds! So, we override library value by our custom one (10s). I believe we must assign lib value in QoSOptions

raman-m · 2024-11-01T18:11:24Z

src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs

-            FailureRatio = 0.8,
-            SamplingDuration = TimeSpan.FromSeconds(10),
+            FailureRatio = options.FailureRatio,
+            SamplingDuration = TimeSpan.FromMilliseconds(options.SamplingDuration),


Both SamplingDuration and BreakDuration are TimeSpans, so they can be initialized from milliseconds.
Not an issue! FYI

raman-m · 2024-11-01T18:23:56Z

src/Ocelot/Configuration/QoSOptions.cs

+            FailureRatio = failureRatio;
+        }
+
+        public QoSOptions(


Why did you add both constructors? I don't see any changes in the Creators' logic.
It appears these constructors were added solely for testing purposes. I would suggest switching to QoSOptionsBuilder and removing these two constructors.
Therefore, you will need to update QoSOptionsBuilder by adding new initialization methods.

raman-m · 2024-11-01T18:26:12Z

src/Ocelot/Configuration/QoSOptions.cs

        /// </value>
+        public int SamplingDuration { get; } = 10000;


I disagree to assign custom default value which differs from the lib one!
So, according to the docs for SamplingDuration, the default value is 30 seconds! So, we override library value by our custom one (10s). I believe we must assign lib value in QoSOptions

raman-m · 2024-11-01T18:29:53Z

src/Ocelot/Configuration/QoSOptions.cs

+        /// <value>
+        /// An <see cref="double"/> ratio of exceptions/requests  (0.8 means 80% failed of all sampled executions).
+        /// </value>
+        public double FailureRatio { get; } = .8;


Actually FailureRatio has the default value is 0.1
So, I don't recommend to override library settings!

raman-m · 2024-11-01T18:31:02Z

src/Ocelot/Configuration/QoSOptions.cs

+        public bool IsValid() =>
+            ExceptionsAllowedBeforeBreaking <= 0 || 
+            ExceptionsAllowedBeforeBreaking >= 2 && DurationOfBreak > 0 && !(FailureRatio <= 0) &&
+            !(FailureRatio > 1) && SamplingDuration > 0;


Is it used for JSON config validation?

raman-m · 2024-11-01T18:33:35Z

test/Ocelot.AcceptanceTests/PollyQoSTests.cs

@@ -38,7 +38,7 @@ public override void Dispose()
    public void Should_not_timeout()
    {
        var port = PortFinder.GetRandomPort();
-        var route = GivenRoute(port, new QoSOptions(10, 500, 1000, null), HttpMethods.Post);
+        var route = GivenRoute(port, new QoSOptions(10, 500, .5, 5, 1000, null), HttpMethods.Post);


Test readability is low❗

We have to switch to QoSOptionsBuilder in tests avoiding direct usage of constructors!
With QoSOptionsBuilder test readability will be excellent. 😉

raman-m · 2024-11-01T18:41:53Z

Raynald, please note that this PR is a successor of #2073, which contains numerous changes to the same logic.

raman-m · 2024-12-26T10:36:41Z

@RaynaldM Ray, could you resolve merge conflicts and code review issues please?

RaynaldM self-assigned this May 29, 2024

RaynaldM added feature A new feature small effort Likely less than a day of development effort. QoS Ocelot feature: Quality of Service aka Polly Configuration Ocelot feature: Configuration labels May 29, 2024

RaynaldM linked an issue May 29, 2024 that may be closed by this pull request

FailureRatio and SamplingDuration parameters of Polly V8 circuit-breaker #2080

Open

raman-m changed the title ~~#2080 Adds parameters (in this case those of Polly V8) to fine-tune c…~~ #2080 New V8 parameters to fine-tune Polly's circuit-breaker behavior May 29, 2024

raman-m changed the title ~~#2080 New V8 parameters to fine-tune Polly's circuit-breaker behavior~~ #2080 Add new V8 params to fine-tune Polly's circuit-breaker behavior May 29, 2024

raman-m added the 2023 Annual 2023 release label Jun 11, 2024

raman-m added this to the Annual 2023 milestone Jun 11, 2024

raman-m force-pushed the 2080-adds-parameters-in-this-case-those-of-polly-v8-to-fine-tune-circuit-breaker-behavior branch from 1de22fd to f69831a Compare June 14, 2024 17:52

raman-m removed the small effort Likely less than a day of development effort. label Jun 14, 2024

raman-m requested review from raman-m and ggnaegi and removed request for raman-m June 19, 2024 16:04

raman-m added Oct'24 October 2024 release and removed 2023 Annual 2023 release labels Aug 12, 2024

raman-m modified the milestones: Annual 2023, September'24 Aug 12, 2024

raman-m mentioned this pull request Oct 15, 2024

#1314 #1869 #2072 Default timeout enhancements #2073

Open

raman-m added Dec'24 December 2024 release and removed Oct'24 October 2024 release labels Oct 26, 2024

raman-m modified the milestones: October'24, Autumn'24 Oct 26, 2024

RaynaldM and others added 7 commits November 1, 2024 20:20

#2080 Adds parameters (in this case those of Polly V8) to fine-tune c…

b6a907f

…ircuit-breaker behavior

arrange doc

e11a1d8

add an isValid method on QoSOptions

auto code reveiw

5eeb24d

arrange doc

7a9057c

add an isValid method on QoSOptions

auto code reveiw

6a746d6

merge

f1b5bc1

Fix style warnings

3199588

raman-m force-pushed the 2080-adds-parameters-in-this-case-those-of-polly-v8-to-fine-tune-circuit-breaker-behavior branch from 8a6926a to 3199588 Compare November 1, 2024 17:56

raman-m requested changes Nov 1, 2024

View reviewed changes

raman-m added the conflicts Feature branch has merge conflicts label Dec 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2080 Add new V8 params to fine-tune Polly's circuit-breaker behavior #2081

#2080 Add new V8 params to fine-tune Polly's circuit-breaker behavior #2081

RaynaldM commented May 29, 2024 •

edited by raman-m

Loading

raman-m commented May 29, 2024 •

edited

Loading

RaynaldM commented May 30, 2024

raman-m commented Jun 11, 2024 •

edited

Loading

raman-m commented Jun 13, 2024

raman-m commented Aug 12, 2024

raman-m left a comment •

edited

Loading

raman-m Nov 1, 2024

raman-m Nov 1, 2024

raman-m Nov 1, 2024

raman-m Nov 1, 2024

raman-m Nov 1, 2024

raman-m Nov 1, 2024

raman-m Nov 1, 2024

raman-m Nov 1, 2024

raman-m commented Nov 1, 2024

raman-m commented Dec 26, 2024

	"FailureRatio": .8 // .8 = 80% of failed, this is default value
	"FailureRatio": .8, // .8 = 80% of failed, this is default value

#2080 Add new V8 params to fine-tune Polly's circuit-breaker behavior #2081

Are you sure you want to change the base?

#2080 Add new V8 params to fine-tune Polly's circuit-breaker behavior #2081

Conversation

RaynaldM commented May 29, 2024 • edited by raman-m Loading

Closes #2080

raman-m commented May 29, 2024 • edited Loading

RaynaldM commented May 30, 2024

raman-m commented Jun 11, 2024 • edited Loading

raman-m commented Jun 13, 2024

raman-m commented Aug 12, 2024

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

No comma❗

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Test readability is low❗

raman-m commented Nov 1, 2024

raman-m commented Dec 26, 2024

RaynaldM commented May 29, 2024 •

edited by raman-m

Loading

raman-m commented May 29, 2024 •

edited

Loading

raman-m commented Jun 11, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading