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

[Docs] Add clarification about property precedence #1918

Merged
merged 18 commits into from
Jan 24, 2024

Conversation

peter-csala
Copy link
Contributor

@peter-csala peter-csala commented Jan 23, 2024

Pull Request

The issue or feature being addressed

#1916

  • I've added some notes about property precedence but not everywhere

Details on the issue fix or feature implementation

  • Added clarification for Timeout about Timeout and TimeoutGenerator
  • Added clarification for CircuitBreaker about BreakDuration and BreakDurationGenerator
  • Added clarification for Hedging about Delay and DelayGenerator
  • Added clarification for Chaos about InjectionRate and InjectionRateGenerator

Extra

  • Added clarification and examples about next delay calculation for retry logic

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

@peter-csala
Copy link
Contributor Author

peter-csala commented Jan 23, 2024

I think it might make sense to add a little description to the retry page about how the following properties contribute in the calculation of the next delay: Delay, DelayGenerator, MaxDelay, UseJitter, and BackoffType

@martintmk, @martincostello Do you agree?

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f40fa29) 84.79% compared to head (622cecc) 84.79%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1918   +/-   ##
=======================================
  Coverage   84.79%   84.79%           
=======================================
  Files         312      312           
  Lines        6893     6893           
  Branches     1056     1056           
=======================================
  Hits         5845     5845           
  Misses        839      839           
  Partials      209      209           
Flag Coverage Δ
linux 84.79% <ø> (?)
macos 84.79% <ø> (ø)
windows ?

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

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

@martintmk
Copy link
Contributor

I think it might make sense to add a little description to the retry page about how the following properties contribute in the calculation of the next delay: Delay, DelayGenerator, MaxDelay, UseJitter, and BackoffType

@martintmk, @martincostello Do you agree?

Sure, sounds like a good idea.

@martincostello martincostello added this to the v8.3.0 milestone Jan 23, 2024
@peter-csala
Copy link
Contributor Author

@martincostello , @martintmk

I've started to document the retry delay calculation algorithm.

I would like to ask your feedback whether is too detailed or is it documented on good level?

Screenshot 2024-01-24 at 10 12 03

@martintmk
Copy link
Contributor

Maybe just give description to each property and how it's affects the calculation and then do a sample calculation for one of the backoff types?

@peter-csala
Copy link
Contributor Author

peter-csala commented Jan 24, 2024

Maybe just give description to each property and how it's affects the calculation and then do a sample calculation for one of the backoff types?

I thought about the same but I soon bumped into the problem that the general description is true only under specific circumstances.

Like:

  • UseJitter: If enabled then it adds a random value between between [-25% of Delay, +25% of Delay]. Except if Backoff is set to Exponential where jitter is always used.
  • MaxDelay: If specified then it caps the delay if the calculated one is greater than this value. Except if you use DelayGenerator where no capping is applied.
  • etc.

Or shall I create a table with three columns: Property, General use, Exception(s)?

docs/strategies/retry.md Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
.github/wordlist.txt Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
- `BackoffType`: It specifies which calculation algorithm should run.
- `Delay`: If only this one is specified then it will be used as is. If others are defined as well then this will be used as a *base delay*.
- `DelayGenerator`: If specified then it will overwrite other properties based calculation. **Except** if it returns a negative `TimeSpan` where the other properties based calculation is used.
- `MaxDelay`: If specified then it caps the delay if the calculated one is greater than this value. **Except** if `DelayGenerator` is used where no capping is applied.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `MaxDelay`: If specified then it caps the delay if the calculated one is greater than this value. **Except** if `DelayGenerator` is used where no capping is applied.
- `MaxDelay`: If specified then it caps the delay if the calculated one is greater than this value. **Except** if `DelayGenerator` is used where no capping is applied.

Just wondering, shall we apply the capping to values produced by generator too?

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 would say yes. That was my intuition before I checked the implementation. But let's deal with that in a separate PR if you don't mind.

Copy link
Contributor

@martintmk martintmk Jan 24, 2024

Choose a reason for hiding this comment

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

My intuition says yes too, the less special cases, the better. The only downside is that sometimes the Retry-After won't be fully respected if the value is greater than MaxDelay.

https://github.com/dotnet/extensions/blob/9f25d9d74674483298cd167cf8ff6e9d9026bf20/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptions.cs#L57

Thoughts @martincostello

Copy link
Member

Choose a reason for hiding this comment

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

I would agree we should apply it - Retry-After isn't necessarily authoritative and the user may wish to try earlier by overriding. If we do that we should make it clear in the documentation.

Copy link
Contributor Author

@peter-csala peter-csala Jan 24, 2024

Choose a reason for hiding this comment

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

It is a concision decision by the strategy configurator that (s)he sets the MaxDelay. If it is set then the strategy should cap the value regardless how it is calculated/retrieved IMHO.

docs/strategies/retry.md Outdated Show resolved Hide resolved

The `BackoffType` property's data type is the [`DelayBackOffType`](xref:Polly.DelayBackOffType) enumeration. This primarily controls how the calculation is done.

### Constant
Copy link
Contributor

@martintmk martintmk Jan 24, 2024

Choose a reason for hiding this comment

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

Suggested change
### Constant
### Example calculation
Below is the example calculation for `Constant` delay type. It showcases how the delay is calculated, the calculation is almost identical for the other (`Linear`, `Exponential`) backoff types.

Or something in that sense.

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've added examples sections for each.

Screenshot 2024-01-24 at 14 49 23
Screenshot 2024-01-24 at 14 49 34
Screenshot 2024-01-24 at 14 49 48

docs/strategies/retry.md Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
docs/strategies/retry.md Outdated Show resolved Hide resolved
@martintmk martintmk merged commit 51044c5 into App-vNext:main Jan 24, 2024
17 checks passed
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.

3 participants