#1875 Use Polly v8 syntax

[RaynaldM]

Closes #1875

• 1875

Proposed Changes

Use Polly 8's specific syntax to define resiliencePipelines (vs. PollyPolicies)

"Polly v8 introduces the concept of resilience pipelines, a powerful tool that blends one or more resilience strategies. This new API foundation echoes the Policy Wrap of previous versions but is now integrated seamlessly into the core API. All strategies start with a pipeline."

in v7, we written

// Create and use the IAsyncPolicy
IAsyncPolicy asyncPolicy = Policy
    .Handle<Exception>()
    .WaitAndRetryAsync(3, _ => TimeSpan.FromSeconds(1));

in v8, it becomes

// Use the ResiliencePipelineBuilder to start building the resilience pipeline
ResiliencePipeline pipeline = new ResiliencePipelineBuilder()
    .AddRetry(new RetryStrategyOptions
    {
        ShouldHandle = new PredicateBuilder().Handle<Exception>(),
        Delay = TimeSpan.FromSeconds(1),
        MaxRetryAttempts = 3,
        BackoffType = DelayBackoffType.Constant
    })
    .Build(); // After all necessary strategies are added, call Build() to create the pipeline.

[raman-m]

This branch is 9 commits ahead of, 3 commits behind develop.

[raman-m]

Ray, build 3064 has failed! Commit https://github.com/ThreeMammals/Ocelot/pull/1914/commits/837590841f3aaf7311336ff6c9787607b01a5eb8 looks like a bad merge 😉 Please fix the build!

[raman-m]

I love when Raynald develops features! 😍

[raman-m]

@RaynaldM I'm very sorry... But we have merge conflicts ❗ Please resolve them, and I'll review.

[RaynaldM]

What more can I do? it works locally

[raman-m]

What more can I do? it works locally

Yes, Sometimes we have a surprise in CircleCI Docker environment. So, having green tests locally doesn't mean the tests will be green in CircleCI build.

[raman-m]

Ray, It is better to rebase feature branches after each release PRs commits delivery to develop. So develop branch has "clean" starting point as merge commit d54836d51f6ea29a2013967e3649a23e19db28b3. Is it possible to rebase this feature branch? I see a lot of PRs merge commits and conflicts resolution... also some Files Changed are highlighted as fake diff. Or will we leave this branch as is?

[raman-m]

@ggnaegi 🆘 Any ideas to fix are welcome! 😉

[raman-m]

@RaynaldM If locally in IDE it works and tests are green, but they are red in CircleCI build then please keep in mind that timeouts on CircleCI should be greater (longer). Try to increase timeout values. CircleCI env is quite unstable, sometimes it can be slow.

@ggnaegi Please help Ray, we need to intensify development of this release, because it is current! 😄

[raman-m]

The build 3415 is 🟢 Ready for code review!

@ggnaegi Let's review it together!

[raman-m]

https://github.com/ThreeMammals/Ocelot/pull/1914/commits/1c30becfc02e9006e2013baf7908ebebd34aa41c Wow! Timings being increased in 10 times helped to fix tests in CircleCI env! 😂

- await Task.Delay(600);
+ await Task.Delay(6000);

But Why in 10 times? Maybe increasing in 2-3 times should help too?!.. LoL! Tears in my eyes :yum:

@RaynaldM 10 times are too much! This is 6 seconds running test!

[raman-m]

@RaynaldM He-he, conflicts! I guess you have to rebase onto develop!

[RaynaldM]

It is a conflict with my own code :)

[raman-m]

Are you going to fix or not? Do you need a help in conflicts resolution?

P.S.

Pay your attention to the fact we have not much time for this release. I want to release on Friday, March 1. But sooner is better.

[raman-m]

Not ready for delivery❗ Moved to the Feb'24 release.

[raman-m]

Look at the build 3540: its running time was increased to 13m 20s. But the running time in develop takes no more 12m ! So, by the tests of this PR we've added extra 1m 20s!!! So, this is too much! We have to review the tests trying to decrease running time.

Update

The next build 3542 took 12m 40s. So, we have extra 40-45 seconds, at least

[raman-m]

The build 3567 has added extra +34s to the duration. Will try to decrease timeouts of the tests...

[raman-m]

@ggnaegi Welcome to review!

[raman-m]

Latest merge commits auto-removed actual version of OcelotBuilderExtensions !!! I am shocked! 🤯 All XML docs were lost! That's why it is better to rebase the branches after each release. But this feature branch looks ugly...

[raman-m]

TODO

• [x] Re-add lost XML docs from commit https://github.com/ThreeMammals/Ocelot/commit/0404c2473d2f1b51849fcedb1512b8a86012b867; ✔️ commit https://github.com/ThreeMammals/Ocelot/pull/1914/commits/5ebb165e2e866153eedf8da993c9a188e413cea3
• [x] Deep code review in VS; ✔️ commit https://github.com/ThreeMammals/Ocelot/pull/1914/commits/e2306ace2429fb5e609ecaa1357954d0f7687249

[raman-m]

@ggnaegi

What is that? 🤯 https://github.com/ThreeMammals/Ocelot/blob/5a3c55e358afb7853819e3370daad9bb15452dd9/src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs#L30 You've injected IHttpContextAccessor object as the _contextAccessor private member and then you get another service object from service collection? https://github.com/ThreeMammals/Ocelot/blob/5a3c55e358afb7853819e3370daad9bb15452dd9/src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs#L17-L25 What was a problem to inject IPollyQoSProvider<HttpResponseMessage> object into the constructor? Was some design issue to inject IPollyQoSProvider<HttpResponseMessage> directly? I just cannot get it?! 😮

---

I expect the design with the constructor like this

 public PollyPoliciesDelegatingHandler( 
     DownstreamRoute route, 
     IPollyQoSProvider<HttpResponseMessage> provider, // Our lovely provider from DI
     IOcelotLoggerFactory loggerFactory) 
 { 
     _route = route; 
     _provider = provider; // !!! Injecting what we want directly
     _logger = loggerFactory.CreateLogger<PollyPoliciesDelegatingHandler>(); 
 } 

and further code:

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        var policy = _provider.GetPollyPolicyWrapper(_route).AsyncPollyPolicy; // !!! _provider is private readonly member

        return await policy.ExecuteAsync(async () => await base.SendAsync(request, cancellationToken));
    }

[raman-m]

🚀 ~50 commits

🙈 and will be more

[raman-m]

@ggnaegi Please review and approve! I believe the PR is ready for delivery. I've got necessary explanations by Ray, and I have a good clarity now. Seems, nothing to improve.

[ggnaegi]

@ggnaegi Please review and approve! I believe the PR is ready for delivery. I've got necessary explanations by Ray, and I have a good clarity now. Seems, nothing to improve.

@raman-m Ok, but it's going to be late, around 11 your time.

[raman-m]

What I'm worrying now is increased duration of running unit tests by ~30s ! Locally it takes 27 seconds to run new 20 unit tests: So, our CircleCI builds will take not 12m but 12m 30s ❗ Last build took 12m 45s! Seems nothing to do with that. But in future we could move all package tests to separate testing project and release the package independently from Ocelot.nupkg

But acceptance tests cannot be moved unfortunately, and they will stay in the current testing project.

[ggnaegi]

@raman-m commented on Mar 9:

Seems nothing to do with that. But in future we could move all package tests to separate testing project and release the package independently from Ocelot.nupkg

Yes you're right!

[ggnaegi]

@raman-m commented on Mar 6

I don't know, probably some design issues since I agree with you about the HttpContextAccessor. Probably the way the object is instantiated.

[raman-m]

@ggnaegi commented on Mar 9

The problem is that your design of the delegate was reused in the new delegate class by Raynald. So, seems it is a little bit late to fix design issue doing some refactoring. But we can skip it for now, and let's keep in mind that this design issue can be fixed in future PR...

[ggnaegi]

@ggnaegi commented on Mar 9

The problem is that your design of the delegate was reused in the new delegate class by Raynald. So, seems it is a little bit late to fix design issue doing some refactoring. But we can skip it for now, and let's keep in mind that this design issue can be fixed in future PR...

@raman-m yes but I think it's quite deeply rooted in the Ocelot's design somewhere. You know, the kind of of classes that are not injected with DI.

[raman-m]

@ggnaegi commented on Mar 9

Great! We have the consensus! So, at least after the release I will create a TODO task to start brainstorming how to separate Polly Provider package to a separate Visual Studio solution with its own testing project. Possibly we could brainstorm how to adjust release process also.

TODO

• Move Ocelot.Provider.Polly package to a separate solution

[ggnaegi]

@ggnaegi commented on Mar 9

Great! We have the consensus! So, at least after the release I will create a TODO task to start brainstorming how to separate Polly Provider package to a separate Visual Studio solution with its own testing project. Possibly we could brainstorm how to adjust release process also.

TODO

• Move Ocelot.Provider.Polly package to a separate solution

@raman-m, I don't think that's the problem. The problem is more related with the fact that the QoS classes don't know the polly provider, since it's provided in a project that is not referenced in ocelot base. That's why I finally used this ugly hack.

[raman-m]

@ggnaegi Cherry pick 0ca4b188cf68b246df80df8ffe86e2286c352366 once again?

[raman-m]

@ggnaegi I expect your fixing code review issues and approving finally till Friday to allow Raynald to merge the PR on Friday, March 15. 🆗 ❔

[raman-m]

@RaynaldM Ready for delivery! ✅

• Code review ✔️✔️
• Team approvals ✔️
• Unit testing ✔️
• Acceptance tests ❔ updated
• Updated docs ✔️