-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Simmy v8 #1459
Simmy v8 #1459
Conversation
…strategies will inherit from * add LatencyChaosStrategy Note: this is still work in progress, I'm just getting familiar with the v8 implemenation.
…rate and enabled options * refactor MonkeyStrategy so that it is instantiated out of options object rather than individual parameters * expose ShouldInject method in the MonkeyStrategy so that consumers who extend/inherit it can also use it for convenience * add Latency property to the LatencyStrategyOptions as a primitive value to not force consumers to always pass a generator * refactors the LatencyChaosStrategy so that the code it's wrap inside a try/catch to handle canceled operations, since the execution can be also canceled within the ShouldInject method or even within the ExecuteCallbackSafeAsync
…s the logic to handle a callback of type T vs TResult to make it easier and clear to consumers, and avoid confusion so that they don't have to worry about it.
* refactor and clean up
…end generic chaos strategies?
# Conflicts: # src/Polly.Core/Utils/OutcomeResilienceStrategy.cs
src/Polly.Core/Simmy/Latency/LatencyChaosPipelineBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Polly.Core/Simmy/Behavior/BehaviorChaosPipelineBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Polly.Core/Simmy/Latency/LatencyChaosPipelineBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Polly.Core/Simmy/Outcomes/OutcomeChaosPipelineBuilderExtensions.TResult.cs
Outdated
Show resolved
Hide resolved
@martintmk 100% of code coverage! I think we can merge this guy now and I'll continue working on any other feedback, docs, examples etc, in another PR, so that @clpun and @joperezr can continue working on the dotnet extensions release. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1459 +/- ##
==========================================
+ Coverage 83.99% 84.63% +0.63%
==========================================
Files 280 306 +26
Lines 6548 6820 +272
Branches 1020 1044 +24
==========================================
+ Hits 5500 5772 +272
Misses 839 839
Partials 209 209
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
I agree we can merge and based on how the API review goes hide new APIs upon release. Btw, there are still issues with tests as 4 mutants survived with your changes. You need to add additional tests to kill them: You can find mutation report here: |
This is awesome, special thanks to @vany0114 and @martintmk for all your hard work here! |
Migrates Simmy to use the new Polly v8
This PR migrates all the Simmy functionality into the new Polly v8 version so that Simmy can also take advantage of all the improvements made in the v8 version.
Details on the issue fix or feature implementation
Inject
prefix from the name, e.g:InjectBehaviorPolicy
->BehaviorChaosStrategy
ChaosStrategy
suffix rather thanResilienceStrategy
to make it clearerBehaviorChaosStrategy
LatencyChaosStrategy
OutcomeChaosStrategy
: this one provides two functionalities, inject faults as well as custom results.MonkeyStrategy
implementation, which is also exposed so that people can extend it to create their own chaos strategies.TODO:
Questions:
AddChaosStrategy
service collection extension to separate it fromAddResilienceStrategy
so that is clearer for the consumers and also to allow keeping them separate in different registries, etc? If so I guess this would require some extra effort to potentially use aCompositeChaosStrategyBuilder
and/or generic constraints.Confirm the following