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

Adopt Polly V8 in Microsoft.Extensions.Resilience #4084

Merged
merged 13 commits into from
Jun 28, 2023
Merged

Conversation

martintmk
Copy link
Contributor

@martintmk martintmk commented Jun 16, 2023

This PR adopts the new Polly.Extensions package.

All the features introduced in Microsoft.Extensions.Resilience are now built into Polly V8. This makes most of the code in this library redundant.

The library will now only expose resilience enrichment and some extensions for ResilienceContext.

var services = new ServiceCollection();
services.AddResilienceEnrichment();

API changes:

Old API

services
    .AddResiliencePipeline<string>("my-pipeline")
    .AddRetryPolicy("my-retry", options =>
    {
        options.RetryCount = 3;
        options.BaseDelay = TimeSpan.FromSeconds(5);
    })
    .AddTimeoutPolicy("my-timeout", options =>
    {
        options.TimeoutInterval = TimeSpan.FromSeconds(30);
    });

New Polly V8 API:

new ServiceCollection()
    .AddResilienceStrategy("my-strategy", builder => builder
    .AddRetry(new RetryStrategyOptions
    {
        StrategyName = "my-retry",
        RetryCount = 3,
        BaseDelay = TimeSpan.FromSeconds(1)
    })
    .AddTimeout(new TimeoutStrategyOptions
    {
        Timeout = TimeSpan.FromSeconds(30),
        StrategyName = "my-timeout"
    }));

The PR won't be mergeable until we adopt V8 in Microsoft.Extensions.Http.Resilience

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned martintmk Jun 16, 2023
@martintmk
Copy link
Contributor Author

cc @martincostello

@martincostello
Copy link
Member

😍
image

_exceptionSummarizer = exceptionSummarizer;
}

public void Enrich(EnrichmentContext context)
Copy link
Member

Choose a reason for hiding this comment

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

Does context need a null check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it will never be null. Polly infra makes sure of that.

Copy link
Member

Choose a reason for hiding this comment

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

Adding _ = Throw.IfNull(context); shouldn't hurt.

Copy link
Member

Choose a reason for hiding this comment

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

We generally reserve Throw.IfNull for public methods. Internally within the context of an assembly, we have the C# compiler's null rules that protect us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the validation is arbitrary as the null won't ever come here.

/// <param name="services">The services.</param>
/// <returns>The input <paramref name="services"/>.</returns>
/// <remarks>
/// This method adds additional dimensions on top of the default ones that are built-in to Polly library. These include:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This method adds additional dimensions on top of the default ones that are built-in to Polly library. These include:
/// This method adds additional dimensions on top of the default ones that are built-in to the Polly library. These include:

/// </summary>
/// <param name="context">The context instance.</param>
/// <returns>The instance of <see cref="RequestMetadata"/> or <see langword="null"/>.</returns>
/// <exception cref="ArgumentNullException">Thrown when <paramref name="context"/> is <see langword="null"/>.</exception>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <exception cref="ArgumentNullException">Thrown when <paramref name="context"/> is <see langword="null"/>.</exception>
/// <exception cref="ArgumentNullException"><paramref name="context"/> is <see langword="null"/>.</exception>

/// <param name="services">The services.</param>
/// <param name="configure">The configure result dimensions.</param>
/// <returns>The input <paramref name="services"/>.</returns>
/// <exception cref="ArgumentNullException">Thrown when <paramref name="services"/> or <paramref name="configure"/> is <see langword="null"/>.</exception>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <exception cref="ArgumentNullException">Thrown when <paramref name="services"/> or <paramref name="configure"/> is <see langword="null"/>.</exception>
/// <exception cref="ArgumentNullException"><paramref name="services"/> or <paramref name="configure"/> is <see langword="null"/>.</exception>

_exceptionSummarizer = exceptionSummarizer;
}

public void Enrich(EnrichmentContext context)
Copy link
Member

Choose a reason for hiding this comment

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

Adding _ = Throw.IfNull(context); shouldn't hurt.

@RussKie
Copy link
Member

RussKie commented Jun 19, 2023

@martintmk the new packages must be available in dotnet-public. Here's the instruction to get those there.

@RussKie RussKie added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Jun 19, 2023
@martintmk
Copy link
Contributor Author

I'll leave it here in draft for a while. I need to work on alpha2 to address some issues first.

@ghost ghost removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Jun 19, 2023
@joperezr joperezr requested a review from geeknoid June 20, 2023 16:45
@joperezr
Copy link
Member

@martintmk is this now superseded by #4108? If so, can we go ahead and close this one?

@joperezr joperezr added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Jun 23, 2023
@martintmk
Copy link
Contributor Author

@martintmk is this now superseded by #4108? If so, can we go ahead and close this one?

@joperezr

This PR is for Microsoft.Extensions.Resilience while the other is for Microsoft.Extensions.Http.Resilience.

I'll continue on this one once the #4108 is merged.

@ghost ghost removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Jun 26, 2023
@martintmk martintmk marked this pull request as ready for review June 27, 2023 14:27
@martintmk
Copy link
Contributor Author

martintmk commented Jun 28, 2023

@RussKie , @geeknoid

Now I am getting the following code-coverate error, even though I haven't touched these projects:

image

Any idea how to move forward? Is it ok to decrease the code-coverage threshold?

@geeknoid
Copy link
Member

We're still fine-tuning the coverage stuff, so yes it's fine to decrease the coverage numbers.

@martintmk martintmk enabled auto-merge (squash) June 28, 2023 14:57
@martintmk martintmk disabled auto-merge June 28, 2023 14:58
@martintmk martintmk enabled auto-merge (squash) June 28, 2023 15:00
@martintmk martintmk merged commit 5562400 into main Jun 28, 2023
6 checks passed
@martintmk martintmk deleted the mtomka/adopt-polly-v8 branch June 28, 2023 15:32
@ghost ghost added this to the 8.0 Preview7 milestone Jun 28, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants