[Docs] Unify strategy descriptions and add Telemetry sections

[peter-csala]

Pull Request

The issue or feature being addressed

I have found the strategies documentation paged a bit inconsistent:

• The About section sometimes contains a short description while other times doesn't.
• I have found some of the description fields of the Defaults sections rather vague.

Details on the issue fix or feature implementation

• Unified Timeout + added Telemetry
• Unified Retry + added Telemetry
• Unified Fallback + added Telemetry
• Unified Rate Limiter + added Telemetry
• Unified Hedging + added Telemetry
• Unified Circuit Breaker + added Telemetry

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.69%. Comparing base (4f67189) to head (899a5c4).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2060      +/-   ##
==========================================
+ Coverage   83.67%   83.69%   +0.01%     
==========================================
  Files         312      312              
  Lines        7106     7114       +8     
  Branches     1054     1054              
==========================================
+ Hits         5946     5954       +8     
  Misses        789      789              
  Partials      371      371              

Flag	Coverage Δ
linux	?
macos	?
windows	83.69% <ø> (+0.01%)	⬆️

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.

[peter-csala]

@martintmk Is there any particular reason why the RetryAfter is not passed to the OnRateLimiterRejectedArguments?

https://github.com/App-vNext/Polly/blob/main/src/Polly.RateLimiting/RateLimiterResilienceStrategy.cs#L41

TimeSpan? retryAfter = null;

if (lease.TryGetMetadata(MetadataName.RetryAfter, out TimeSpan retryAfterValue))
{
    retryAfter = retryAfterValue;
}

var args = new OnRateLimiterRejectedArguments(context, lease);
_telemetry.Report(new(ResilienceEventSeverity.Error, RateLimiterConstants.OnRateLimiterRejectedEvent), context, args);

if (OnLeaseRejected != null)
{
    await OnLeaseRejected(new OnRateLimiterRejectedArguments(context, lease)).ConfigureAwait(context.ContinueOnCapturedContext);
}

var exception = retryAfter.HasValue ? new RateLimiterRejectedException(retryAfter.Value) : new RateLimiterRejectedException();

return Outcome.FromException<TResult>(exception.TrySetStackTrace());

[martintmk]

Is there any particular reason why the RetryAfter is not passed to the OnRateLimiterRejectedArguments?

Frankly, I don't know :D

It was probably just an omission as we didn't have any use-case where this would be required. If there is a real-world use-case we can add it there.

[martintmk]

One thing I am wondering is to add Telemetry section with the name of the events that each resilience strategy produces. It can also point to proper telemetry page in case someone needs more details.

https://www.pollydocs.org/advanced/telemetry.html#metrics

[peter-csala]

One thing I am wondering is to add Telemetry section with the name of the events that each resilience strategy produces. It can also point to proper telemetry page in case someone needs more details.

https://www.pollydocs.org/advanced/telemetry.html#metrics

Good idea, I'll do that.

[peter-csala]

@martintmk Do I see it correctly that this transitionedState variable can be removed from here?

https://github.com/App-vNext/Polly/blob/main/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs#L337

[martincostello]

Do I see it correctly that this transitionedState variable can be removed from here?

Looks like some leftovers from a refactor to me.

[peter-csala]

@martincostello , @martintmk How do you like it?

[peter-csala]

@martintmk I've just realized that the Execution Time is reported without the unit in case of Execution Attempt.

I know it is mentioned in the documentation that the unit is millisecond, but I would rather prefer self-explanatory events.

Expected

Execution attempt. Source: '(null)/(null)/Hedging', Operation Key: '', Result: '1',
 Handled: 'False', Attempt: '0', Execution Time: '1505.3839 ms'

Actual

Execution attempt. Source: '(null)/(null)/Hedging', Operation Key: '', Result: '1', 
 Handled: 'False', Attempt: '0', Execution Time: '1505.3839'

Same applies for PipelineExecuting and PipelineExecuted events.

Can we fix these in a separate PR?

[martintmk]

@martintmk I've just realized that the Execution Time is reported without the unit in case of Execution Attempt.

I know it is mentioned in the documentation that the unit is millisecond, but I would rather prefer self-explanatory events.

Expected

Execution attempt. Source: '(null)/(null)/Hedging', Operation Key: '', Result: '1',
 Handled: 'False', Attempt: '0', Execution Time: '1505.3839 ms'

Actual

Execution attempt. Source: '(null)/(null)/Hedging', Operation Key: '', Result: '1', 
 Handled: 'False', Attempt: '0', Execution Time: '1505.3839'

Same applies for PipelineExecuting and PipelineExecuted events.

Can we fix these in a separate PR?

Sure, makes sense. Just don't put the ms formatting string so it isn't included in value when structured logging is used.

[martintmk]

just few nits, looks good otherwise