Skip to content

[Docs] Unify strategy descriptions and add Telemetry sections - #2060

Merged
martincostello merged 30 commits into
App-vNext:mainfrom
peter-csala:unify-strategy-descriptions
Apr 22, 2024
Merged

[Docs] Unify strategy descriptions and add Telemetry sections#2060
martincostello merged 30 commits into
App-vNext:mainfrom
peter-csala:unify-strategy-descriptions

Conversation

@peter-csala

@peter-csala peter-csala commented Apr 15, 2024

Copy link
Copy Markdown
Contributor

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 Bot commented Apr 15, 2024

Copy link
Copy Markdown

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.

Comment thread docs/strategies/retry.md Outdated
Comment thread docs/strategies/retry.md Outdated
Comment thread docs/strategies/retry.md Outdated
Comment thread docs/strategies/retry.md Outdated
Comment thread docs/strategies/retry.md Outdated
Comment thread docs/strategies/timeout.md Outdated
Comment thread docs/strategies/timeout.md Outdated
Comment thread docs/strategies/timeout.md Outdated
@peter-csala

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

Comment thread docs/strategies/rate-limiter.md Outdated
Comment thread docs/strategies/rate-limiter.md Outdated
Comment thread docs/strategies/rate-limiter.md Outdated
Comment thread docs/strategies/rate-limiter.md Outdated
Comment thread docs/strategies/rate-limiter.md Outdated
peter-csala and others added 3 commits April 17, 2024 12:06
@peter-csala

Copy link
Copy Markdown
Contributor Author

@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

Comment thread docs/strategies/circuit-breaker.md Outdated
Comment thread docs/strategies/circuit-breaker.md Outdated
Comment thread docs/strategies/circuit-breaker.md Outdated
Comment thread docs/strategies/circuit-breaker.md Outdated
Comment thread docs/strategies/circuit-breaker.md Outdated
peter-csala and others added 2 commits April 17, 2024 16:11
Co-authored-by: Martin Costello <martin@martincostello.com>
@martincostello

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

@martincostello , @martintmk How do you like it?

Screenshot 2024-04-18 at 10 54 12

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