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.

Sign up for GitHub

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

Jump to bottom

More open customization of Polly use #1844

Closed
RaynaldM opened this issue Dec 7, 2023 · 2 comments · Fixed by #1845 or #1974
Closed

More open customization of Polly use #1844

RaynaldM opened this issue Dec 7, 2023 · 2 comments · Fixed by #1845 or #1974
Assignees
@RaynaldM
Labels
feature A new feature Jan'24 January 2024 release merged Issue has been merged to dev and is waiting for the next release
Milestone
January'24

Comments

@RaynaldM
Copy link
Collaborator

RaynaldM commented Dec 7, 2023 •
edited by raman-m

In certain contexts, we need to be able to fully tune the way Polly is used for timeouts and circuit-breakers, but not only that.
With this in mind, I'm proposing in this PR to open up the use of AddPolly by adding :

public static IOcelotBuilder AddPolly<T>(this IOcelotBuilder builder, Dictionary<Type, Func<Exception, Error>> errorMapping)
     where T : class, IPollyQoSProvider<HttpResponseMessage> =>
     AddPolly<T>(builder, GetDelegatingHandler, errorMapping);

 public static IOcelotBuilder AddPolly<T>(this IOcelotBuilder builder, QosDelegatingHandlerDelegate delegatingHandler)
     where T : class, IPollyQoSProvider<HttpResponseMessage> =>
     AddPolly<T>(builder, delegatingHandler, ErrorMapping);

 public static IOcelotBuilder AddPolly<T>(this IOcelotBuilder builder)
     where T : class, IPollyQoSProvider<HttpResponseMessage> =>
     AddPolly<T>(builder, GetDelegatingHandler, ErrorMapping);

Thanks to this, we will be able to use our own implementations of IPollyQoSProvider<HttpResponseMessage>, QosDelegatingHandlerDelegate and Dictionary<Type, Func<Exception, Error>>
@RaynaldM RaynaldM self-assigned this Dec 7, 2023
@RaynaldM RaynaldM added feature A new feature small effort Likely less than a day of development effort. 2023 Annual 2023 release labels Dec 7, 2023
RaynaldM pushed a commit that referenced this issue Dec 7, 2023
#1844 More open customization of Polly use
b223f7c
@RaynaldM RaynaldM linked a pull request Dec 7, 2023 that will close this issue
#1844 More open customization of Polly use #1845
Merged
@raman-m
Copy link
Member

raman-m commented Dec 11, 2023

Nice feature! 👍

@raman-m raman-m added the accepted Bug or feature would be accepted as a PR or is being worked on label Dec 11, 2023
@raman-m raman-m added Jan'24 January 2024 release and removed 2023 Annual 2023 release labels Feb 17, 2024
@raman-m raman-m added this to the January'24 milestone Feb 17, 2024
raman-m pushed a commit that referenced this issue Feb 17, 2024
@raman-m
#1844 More open customization of Polly use
9f7a724
raman-m pushed a commit that referenced this issue Feb 23, 2024
@raman-m
#1844 More open customization of Polly use
57d72ce
RaynaldM pushed a commit that referenced this issue Feb 23, 2024
@RaynaldM
#1844 More open customization of Polly use
8b15c89
RaynaldM pushed a commit that referenced this issue Feb 23, 2024
@RaynaldM
#1844 More open customization of Polly use
a5a675a
RaynaldM added a commit that referenced this issue Feb 23, 2024
@RaynaldM
Merge pull request #1845 from ThreeMammals/1844-more-open-customizati…
f88bd51 
…on-of-polly-use

#1844 More open customization of Polly use
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Feb 23, 2024
@raman-m
Copy link
Member

raman-m commented Feb 25, 2024

Not delivered

@raman-m raman-m reopened this Feb 25, 2024
@raman-m raman-m removed the merged Issue has been merged to dev and is waiting for the next release label Feb 25, 2024
@raman-m raman-m mentioned this issue Feb 25, 2024
Merged
@RaynaldM RaynaldM linked a pull request Feb 26, 2024 that will close this issue
#1844 More open customization of Polly use #1974
Merged
RaynaldM added a commit that referenced this issue Feb 26, 2024
@RaynaldM @raman-m
#1844 More open customization of Polly use (#1974)
0404c24 
* #1844 More open customization of Polly use

* Update src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* #1844 More open customization of Polly use

* Update src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* fix compilation errors

* add xml doc

* More XML-docs

* remove new line separators

---------

Co-authored-by: Ray <rmessie@traceparts.com>
Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed small effort Likely less than a day of development effort. labels Feb 26, 2024
@raman-m raman-m mentioned this issue Mar 4, 2024
Merged
RaynaldM added a commit that referenced this issue Mar 15, 2024
@RaynaldM @raman-m @ggnaegi
#1875 Use Polly v8 syntax (#1914)
d6eefc8 
* #1844 More open customization of Polly use

* First step: implementation done, The UTs have yet to be determined

* reverted changes in PollyPoliciesDelegatingHandlerTests
created tests for  PollyResiliencePipelineDelegatingHandler
created tests for PollyQoSResiliencePipelineProvider

* renaming variables for better clarity in PollyQoSResiliencePipelineProviderTests

* The most significant changes involve updates to the `OcelotBuilderExtensions` and `PollyQoSResiliencePipelineProvider` classes, as well as modifications to the `PollyQoSTests` class.

In the `OcelotBuilderExtensions` class, the `GetDelegatingHandler` method is now used instead of `GetDelegatingHandlerV7` in the `AddPolly` methods. The `AddPolly` method without parameters has been simplified using a lambda expression. The `GetDelegatingHandler` method now returns a new instance of `PollyResiliencePipelineDelegatingHandler` instead of `PollyPoliciesDelegatingHandler`.

In the `PollyQoSResiliencePipelineProvider` class, a comment has been added suggesting the use of `ResiliencePipelineRegistry<TKey>.GetOrAddPipeline` due to its thread-safe nature. The `MinimumThroughput` property in `circuitBreakerStrategyOptions` now uses the value from `route.QosOptions.ExceptionsAllowedBeforeBreaking`.

In the `PollyQoSTests` class, the `QoSOptions` used in the test configurations have been updated. Two tests, `Should_open_circuit_breaker_then_close` and `Open_circuit_should_not_effect_different_route`, have been modified to repeat the same request as the minimum `ExceptionsAllowedBeforeBreaking` is now 2. The `GivenThereIsAPossiblyBrokenServiceRunningOn` method now delays for 2.1 seconds when the request count is 2, to ensure the circuit is open.

Changes:

1. `OcelotBuilderExtensions` class updated to use `GetDelegatingHandler` method.
2. `AddPolly` methods in `OcelotBuilderExtensions` class simplified.
3. `GetDelegatingHandler` method in `OcelotBuilderExtensions` class now returns a new instance of `PollyResiliencePipelineDelegatingHandler`.
4. Comment added in `PollyQoSResiliencePipelineProvider` class suggesting the use of `ResiliencePipelineRegistry<TKey>.GetOrAddPipeline`.
5. `MinimumThroughput` property in `circuitBreakerStrategyOptions` updated to use value from `route.QosOptions.ExceptionsAllowedBeforeBreaking`.
6. `QoSOptions` in `PollyQoSTests` class updated.
7. `Should_open_circuit_breaker_then_close` and `Open_circuit_should_not_effect_different_route` tests in `PollyQoSTests` class updated.
8. `GivenThereIsAPossiblyBrokenServiceRunningOn` method in `PollyQoSTests` class updated to delay for 2.1 seconds when request count is 2.

* end of implementation

* remove "Microsoft.VisualStudio.Azure.Containers.Tools.Targets" packages in sample

* Use `ResiliencePipelineRegistry` in `PollyQoSResiliencePipelineProvider`. This new implementation replaces the previous dictionary-based approach for managing cache for resilience pipelines.

* fix conflict

* end of merge of develop (+polly package uptdate)

* fix polly update

* #1844 More open customization of Polly use

* First step: implementation done, The UTs have yet to be determined

* reverted changes in PollyPoliciesDelegatingHandlerTests
created tests for  PollyResiliencePipelineDelegatingHandler
created tests for PollyQoSResiliencePipelineProvider

* renaming variables for better clarity in PollyQoSResiliencePipelineProviderTests

* The most significant changes involve updates to the `OcelotBuilderExtensions` and `PollyQoSResiliencePipelineProvider` classes, as well as modifications to the `PollyQoSTests` class.

In the `OcelotBuilderExtensions` class, the `GetDelegatingHandler` method is now used instead of `GetDelegatingHandlerV7` in the `AddPolly` methods. The `AddPolly` method without parameters has been simplified using a lambda expression. The `GetDelegatingHandler` method now returns a new instance of `PollyResiliencePipelineDelegatingHandler` instead of `PollyPoliciesDelegatingHandler`.

In the `PollyQoSResiliencePipelineProvider` class, a comment has been added suggesting the use of `ResiliencePipelineRegistry<TKey>.GetOrAddPipeline` due to its thread-safe nature. The `MinimumThroughput` property in `circuitBreakerStrategyOptions` now uses the value from `route.QosOptions.ExceptionsAllowedBeforeBreaking`.

In the `PollyQoSTests` class, the `QoSOptions` used in the test configurations have been updated. Two tests, `Should_open_circuit_breaker_then_close` and `Open_circuit_should_not_effect_different_route`, have been modified to repeat the same request as the minimum `ExceptionsAllowedBeforeBreaking` is now 2. The `GivenThereIsAPossiblyBrokenServiceRunningOn` method now delays for 2.1 seconds when the request count is 2, to ensure the circuit is open.

Changes:

1. `OcelotBuilderExtensions` class updated to use `GetDelegatingHandler` method.
2. `AddPolly` methods in `OcelotBuilderExtensions` class simplified.
3. `GetDelegatingHandler` method in `OcelotBuilderExtensions` class now returns a new instance of `PollyResiliencePipelineDelegatingHandler`.
4. Comment added in `PollyQoSResiliencePipelineProvider` class suggesting the use of `ResiliencePipelineRegistry<TKey>.GetOrAddPipeline`.
5. `MinimumThroughput` property in `circuitBreakerStrategyOptions` updated to use value from `route.QosOptions.ExceptionsAllowedBeforeBreaking`.
6. `QoSOptions` in `PollyQoSTests` class updated.
7. `Should_open_circuit_breaker_then_close` and `Open_circuit_should_not_effect_different_route` tests in `PollyQoSTests` class updated.
8. `GivenThereIsAPossiblyBrokenServiceRunningOn` method in `PollyQoSTests` class updated to delay for 2.1 seconds when request count is 2.

* Use `ResiliencePipelineRegistry` in `PollyQoSResiliencePipelineProvider`. This new implementation replaces the previous dictionary-based approach for managing cache for resilience pipelines.

* end of implementation

* remove "Microsoft.VisualStudio.Azure.Containers.Tools.Targets" packages in sample

* fix conflict

* end of merge of develop (+polly package uptdate)

* fix polly update

* renaming

* fix compile error

* The most significant changes involve the modification of the `errorMapping` type from `Dictionary<Type, Func<Exception, Error>>` to `IDictionary<Type, Func<Exception, Error>>` across multiple files. This change is aimed at enhancing code flexibility and testability. Additionally, the `AddPolly<T>` and `AddPollyV7<T>` methods in `OcelotBuilderExtensions.cs` have been updated to accept `IDictionary` instead of `Dictionary`.

Here are the changes in detail:

1. The type of `errorMapping` was changed from `Dictionary<Type, Func<Exception, Error>>` to `IDictionary<Type, Func<Exception, Error>>` in several places across multiple files to make the code more flexible and easier to test.
2. The `AddPolly<T>` and `AddPollyV7<T>` methods in `OcelotBuilderExtensions.cs` were updated to accept `IDictionary` instead of `Dictionary`.
3. The `_mappers` field and the corresponding assignment in the constructor in `HttpExceptionToErrorMapper.cs` were updated to use `IDictionary`.
4. The `WithExceptionsAllowedBeforeBreaking` and `WithTimeoutValue` values in `OcelotBuilderExtensionsTests.cs` and `PollyQoSResiliencePipelineProviderTests.cs` were updated.
5. The `errorMapping` variable in `HttpExceptionToErrorMapperTests.cs` was updated to use `IDictionary`.

* try to fix UTs on CircleCI

* try another settings

* one more test

* post code reveiew and develop merge

* Quick code review by @raman-m

* Re-add lost XML docs from commit 0404c24
Complete docs

* Update Polly version

* Update Polly to 8.3.1

* Code review by @raman-m

* Arrange file layout in a separate directory (V7)
Modify Obsolete attribute texts
Add note in doc

* Update qualityofservice.rst

Update docs according to the Polly v8 implementations and QoS design upgrade

* More XML docs

* Make the base class abstract

* some code cleanup

* some comments

* remove comments

* Re-use logger and fix warning

* Final review of QoS docs

---------

Co-authored-by: Ray <rmessie@traceparts.com>
Co-authored-by: Tomislav Prebeg <tprebeg@traceparts.com>
Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
Co-authored-by: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com>
raman-m pushed a commit to marklonquist/Ocelot that referenced this issue Apr 7, 2024
@raman-m
ThreeMammals#1844 More open customization of Polly use
5ad0fa2
raman-m pushed a commit to marklonquist/Ocelot that referenced this issue Apr 16, 2024
@raman-m
ThreeMammals#1844 More open customization of Polly use
642fa23
raman-m pushed a commit to marklonquist/Ocelot that referenced this issue Apr 19, 2024
@raman-m
ThreeMammals#1844 More open customization of Polly use
0b2dc77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RaynaldM RaynaldM

Labels
feature A new feature Jan'24 January 2024 release merged Issue has been merged to dev and is waiting for the next release
Projects
None yet
Milestone
January'24
2 participants
@RaynaldM @raman-m