[WIP-DO NOT MERGE] Add InjectFault and InjectLatency features

[mebjas]

The issue or feature being addressed

Support for failure / chaos injection policies to test resilience of the system. Being discussed in details - #499

Details on the issue fix or feature implementation

Introduced a base Monkey Policy along with support for Policy.InjectFault and Policy.InjectLatency as first class citizens in the library.

Confirm the following

• I have merged the latest changes from the dev vX.Y branch
• I have successfully run a local build
• I have included unit tests for the issue/feature
• I have targeted the PR against the latest dev vX.Y branch

[dnfclas]

All CLA requirements met.

[mebjas]

Created this PR to get reviews on the top level functions. Will start working on the tests.

[reisenberger]

@mebjas This is looking great. Thank you very much for all the work on it!

(Review here may look like lots of comments, but mainly it is just a simple few things repeating because of the repetitive Polly syntax model; the concept coded is 👍 .)

[reisenberger]

Func<Context, CancellationToken, Task>

[reisenberger]

Func<Context, CancellationToken, Task<DelegateResult<TResult>>

[mebjas]

Ah this is a todo, I'll add.

[reisenberger]

@mebjas Apart from minor review above we can move to adding tests 👍 (as you say).

For writing deterministic unit tests for the code involving randomisation, an approach could be similar to the way existing Polly tests involving time abstract SystemClock:

• Abstract out the randomiser in the same way Polly already abstracts out SystemClock.
• Tests can then manipulate the abstracted dependency. In this case, tests could replace the randomiser with a deterministic implementation returning fixed values, for specific tests.
• We attribute such test classes to ensure tests which manipulate these ambient contexts do not run in parallel and pollute each other. (Although loss of parallelism of test-running may seem a cost, the time-saving of eliminating real-time delays in wait and timeout tests far compensates.)

(Time fits the ambient-context pattern for the dependency rather than constructor injection or property injection because there is a sensible local default and there are not known good use cases for varying the implementation in production. Similar arguments can apply to randomisation.)

[mebjas]

@reisenberger I have dealt with most of the comments. Had some comments in the Engine implementation for supporting TResult.
About the random function, are you proposing to write the method to return deterministic values during the test with some preprocessor directives?

[reisenberger]

@mebjas Thank you again for your contributions! Re:

About the random function

If we adopt the same strategy as SystemClock, then in RandomGenerator we could have:

public static Func<Double> GetRandomNumber = () => rand.NextDouble();

Tests can then replace the Func<Double> to provide deterministic values eg:

RandomGenerator.GetRandomNumber = () => 0.1;  // If this appears in a test, all calls to RandomGenerator.GetRandomNumber() in that test will return 0.1 

It's important then to add this attribute to classes containing tests which manipulate RandomGenerator.GetRandomNumber in this way, to prevent these tests cross-polluting each other.

And: the test class should be IDisposable with the Dispose() method using a RandomGenerator.Reset() method to reset the GetRandomNumber method.

public static void Reset()
{
    GetRandomNumber = () => rand.NextDouble();
}

(xUnit uses the Dispose() method to clean up after each individual test.)

Overall, this is the same pattern you can see in use with tests manipulating SystemClock.

---

I will aim to check your other qs and the overloads in the coming days.

[mebjas]

@reisenberger pretty neat, thanks for that.

The other question is around usage of DelegateResult<TResult>. Meanwhile, I'll start adding tests.

[mebjas]

@reisenberger I have added decent amount of test cases to cover all syntaxes. Please have a look.

[martincostello]

I was just having a look at this PR out of curiosity, and I wondered is there any argument for the delegates for checking if faults being enabled in the async cases to also have an overload to accept a CancellationToken?

The use case I'm thinking of is for where someone might plumb in something where something external is called out to to determine if things are enabled or not? Of course that might be an edge-case and/or a bad idea, but thought I'd just throw the idea out there.

[mebjas]

@martincostello

I was just having a look at this PR out of curiosity, and I wondered is there any argument for the delegates for checking if faults being enabled in the async cases to also have an overload to accept a CancellationToken?

The use case I'm thinking of is for where someone might plumb in something where something external is called out to to determine if things are enabled or not? Of course that might be an edge-case and/or a bad idea, but thought I'd just throw the idea out there.

Like this - https://github.com/mebjas/Polly/blob/dev-minhazv/src/Polly.Shared/Monkey/MonkeyEngineAsync.cs#L14 ?

[martincostello]

Yeah, like that.

[mebjas]

Yeah, like that.

Yeah problems similar to this was addressed in one or other comments. And was implemented post that.
Let me know if you are doing further review - I'll take care of all your comments / suggestions in a single push.

@martincostello - Thanks!

[mebjas]

@reisenberger AFAIK the fluent assertion tests would be running in parallel.

Even if I set

RandomGenerator.GetRandomNumber = () => 0.1; 

and reset in Dispose() method. It seems different state of tests are interfering with this common static object. How to deal with this here?

[reisenberger]

Hi @mebjas . To force certain tests to not run in parallel, so that they don't cross-pollute:

add this attribute to classes containing tests which manipulate RandomGenerator.GetRandomNumber .

(We should probably rename the attribute to something like: AmbientContextDependentTestCollection.)

[mebjas]

Hi @mebjas . To force certain tests to not run in parallel, so that they don't cross-pollute:

add this attribute to classes containing tests which manipulate RandomGenerator.GetRandomNumber .

(We should probably rename the attribute to something like: AmbientContextDependentTestCollection.)

Added the attribute. The attribute is being used in a lot of places. Should it be renamed in this PR?

[reisenberger]

Added the attribute. The attribute is being used in a lot of places. Should it be renamed in this PR?

Great, thanks! I am happy if you want to leave the renaming to me as a clean-up pass after we have finished all the changes for v6.2.0.

[bartelink]

Sounds great

• separate libs FTW (and making sure the path for wrapper libs is smooth and continues to be is great for the ecosystem in general (plug: https://github.com/jet/CallPolly)). Muchos kudos for taking on the stacks of extra work in the name of doing the right thing @reisenberger
• Simmy works best for me naming-wise (I guess Simmy is a monkey-pirate with a parrot on it's shoulders in the logo?)
• I agree with doing a merge with immediate demerge even if the guys are too humble
• New levels of hierarchy such as MonkeyPolicy and CustomPolicy really need to earn their keep as they start with -100 points for consumers and people walking the code; I'd favor having a common interface (if necessary) but not a common type until it really hurts (I could be missing something and don't have time to dig deep as to whether this even makes sense in practice.)

[vany0114]

@reisenberger sounds great!

• I totally agree with the creation of a new package.
• I like Simmy but definitively I love Molly.
• See my name in Polly's contributors would make me happy and proud 😃
• I could help with the creation of the new repo, etc since next week, I'll be on vacations leave 🎉

Thanks @reisenberger for all your extra work on this!

[mebjas]

@reisenberger

• Makes sense to me too. +1 on this;
• Simmy name sounds cool :) @bartelink Nice logo idea; another derivation from this could be a parrot with eye patch. Ah, nvm maybe too early for this.
• Totally agree with following the similar interface suggestion. Would this library have a dependence on Polly? I guess yes!
• Thanks for merge, de-merge.
• I'd like to contribute both idea wise and code wise to this new project, kindly let me know where all the discussions happen.
• Also, I am in if you need any help with the new project.

Thanks @reisenberger @vany0114 @bartelink

[reisenberger]

Thank you @mebjas @vany0114 @bartelink . Courtesy update: I am likely to next get a chunk of time to progress, in late December.

[reisenberger]

@mebjas @vany0114 I completed an extensive refactor of Polly (#552) to allow us to host policies outside Polly in a new manner. I need to set up the new Simmy/Molly repo, then we can pull the code over there, and we can all pitch in on any final code clean-up and documentation!

[mebjas]

Thanks for this!

Is there any documentation on how this hosting policies outside polly looks like?

[reisenberger]

Is there any documentation on how this hosting policies outside polly looks like?

I have rebased the PR onto v7.0.0 branch, and am aiming to push a commit to show how the policies would join to the new v7.0.0 approach.

Full documentation (for the wider user community) will also follow.

[reisenberger]

@mebjas @vany0114 I was unable to push further changes to this PR, being today's modifications to bring everything into line with v7.0.0. (Maybe we have rebased the PR too many times ... not digging further now.)

I have opened a separate PR #553 as a mechanism only to show you latest modifications:

• Tie MonkeyPolicies into new way of integrating custom policies, Polly v7.0.0
• Classes InjectBehaviourPolicy, InjectOutcomePolicy and InjectLatencyPolicy, to fit this new pattern
• Move syntax to MonkeyPolicy.InjectFault(...)
• Move syntax to MonkeyPolicy.InjectLatency(...)
• Move syntax to MonkeyPolicy.InjectBehaviour(...)
• Adjust specs for above
• Tests for functionality change: make it valid for InjectFault policies to dynamically inject null (no fault)
• Tests for new guard conditions: fault-injection thresholds being out of range

---

Thanks for the huge amount of work you did @mebjas @vany0114 to get PR 508 to functionality-ready. My last piece here bridges us to a way forward with Simmy as a separate package to Polly v7; and implements the syntax preferences described in the consulation document.

Any comment welcome.

Next steps (probably not before next weekend) : Set up the new Simmy repo and build; and move the code over to it.

Given the syntax preferences are done, I think the code as #553 is ready to go (in the new repo), bar trivial re-namespacing (namespace Simmy). Will be good to get all your hard work released to the Polly public! 👍

[reisenberger]

@mebjas @vany0114 Note: The state that #508 has got into due to repeated re-basing/upward merges, means we may not be able/be wise to do the merge/de-merge from to master on Polly (I'm not that happy about risking the state of the Polly master branch from this PR ...), but we will ensure that you get credit for this work all over the doco. Thanks!

[vany0114]

Thanks for all your work on this PR, looking forward to see Polly v7 and Simmy!

[mebjas]

@reisenberger I can see a lot of efforts were put into the redesign and it looks much more polished. Thanks for the contribution :)

Look forward to the new repo and adaptation.

[reisenberger]

@vany0114 @mebjas , Courtesy update: I am working on the Polly v7.0 user documentation (custom policy documentation) which we need for launching v7 (which we need launched, in turn, for Simmy to reference Polly from the external repo).

[mebjas]

@vany0114 @mebjas , Courtesy update: I am working on the Polly v7.0 user documentation (custom policy documentation) which we need for launching v7 (which we need launched, in turn, for Simmy to reference Polly from the external repo).

Thanks @reisenberger
Where are the documentations maintained? Is there a data for v7?

[vany0114]

@vany0114 @mebjas , Courtesy update: I am working on the Polly v7.0 user documentation (custom policy documentation) which we need for launching v7 (which we need launched, in turn, for Simmy to reference Polly from the external repo).

Thanks @reisenberger! If you need some help, just let me know.

[reisenberger]

@mebjas @vany0114 / anyone interested: I created the new repo for Simmy! Please head over! If you are interested to take up any of the items marked help-wanted, that will speed us all along! (Post on the issue to say anything you are picking up ...). Thanks!

I am half-way thru the custom policy documentation, and will check in with something for all to read (and comment) soon as I can. Thanks!

[vany0114]

Guys, I picked this one up for now

[reisenberger]

Hi @mebjas Re:

Where are the documentations maintained?

The Readme and Wiki of this repo. We also sometimes blog.

Is there a data for v7?

The readme directs people to sources of info on changes

[reisenberger]

Locking this thread so that conversation can relate to the latest issues and version of the codebase on the Simmy repo; interested users should head over there!

If anyone has concerns/questions about anything mentioned higher up this thread, please do mention it on Simmy or the design thread here on Polly.

[reisenberger]

Closing as this codebase now exists definitively in the Simmy repo, the readme now links out to Simmy and #499 still tracks on the issues page.