Simmy API Review and Feedback

[martintmk]

I was playing with the new chaos strategies and I think we can simplify the setup of common faults by introducing something similar to PredicateBuilder, but this time for faults.

Instead of:

new ResiliencePipelineBuilder<string>()
    .AddChaosFault(new()
    {
        FaultGenerator = _ =>
        {
            Exception? ex = Random.Shared.Next(380) switch
            {
                < 100 => new InvalidOperationException(),
                < 180 => new HttpRequestException(),
                < 280 => new TimeoutException(),
                < 380 => new TimeoutException(),
                _ => null,
            };

            return ValueTask.FromResult(ex);
        }
    })
    .AddChaosResult(new()
    {
        OutcomeGenerator = _ =>
        {
            Outcome<string>? result = Random.Shared.Next(420) switch
            {
                < 100 => Outcome.FromException<string>(new InvalidOperationException()),
                < 200 => Outcome.FromException<string>(new HttpRequestException()),
                < 300 => Outcome.FromException<string>(new TimeoutException()),
                < 320 => Outcome.FromException<string>(new SocketException()),
                < 420 => Outcome.FromResult("Hello World"),
                _ => null,
            };

            return ValueTask.FromResult(result);
        }
    });

We can allow this simplified API for common scenarios:

new ResiliencePipelineBuilder<string>()
    .AddChaosFault(new()
    {
        FaultGenerator = new FaultGenerator()
            .AddFault<InvalidOperationException>() // default weight is 100
            .AddFault<HttpRequestException>(weight: 80)
            .AddFault<TimeoutException>()
            .AddFault<SocketException>()
    })
    .AddChaosResult(new()
    {
        OutcomeGenerator = new OutcomeGenerator<string>()
            .AddException<InvalidOperationException>() // default weight is 100
            .AddException<HttpRequestException>()
            .AddException<TimeoutException>()
            .AddException<SocketException>(weight: 20)
            .AddResult(() => "Hello World!")
    });

We can also use this issue for additional API feedback.

cc @martincostello , @vany0114

[peter-csala]

@martintmk Don't we want to covert this issue to something like Simmy API review?

I have a couple of observations as well. Just to name a few:

• Enabled's default value is false. I can guarantee that many people will 🤦🏻 when they realize that the missing piece is Enabled = true
• OnXYZ names are inconsistent, like OnFaultInjected, OnLatency, etc.
• Inconsistent API: each chaos has XYZGenerator delegate except Latency chaos which has Latency as well.
• etc.

[martintmk]

@martintmk Don't we want to covert this issue to something like Simmy API review?

Done.

Also, I agree on your points (Enabled, some inconsistencies, etc.) . Feel free to API PR to address those and just reference this issue.

Inconsistent API: each chaos has XYZGenerator delegate except Latency chaos which has Latency as well.

This was intentional, for strategies that produce exceptions and results we don't want to return cached value, but always a new instance. That's why we don't have that field for fault and outcome chaos strategies.

[martintmk]

I'll prepare the PR for FaultGenerator and OutcomeGenerator<T> during the weekend once I have some free time.

[vany0114]

@martintmk that would be a nice enhancement!

[peter-csala]

@martintmk, @martincostello We have a live documentation to an unreleased set of features. I think it might make sense to add a banner at the top of the chaos pages to inform the reader that it will be soon released. What do you guys think?

[martincostello]

Alternatively/additionally, just add somewhere that the features are part of Polly 8.3.0, which users will find doesn't exist yet.

[vany0114]

@peter-csala BTW the link to Simmy in the Chaos index page is pointing to the Polly-Contrib repo. Also, we should make the announcement in the old repo (PollyContrib/Simmy) and pin it once Simmy is live, I can do so.

[vany0114]

@martincostello @martintmk I also think we should include some documentation in the readme as GitHub is the main point of contact so if there's nothing about Simmy in the repo's readme Simmy might be overlooked. We could create a readme file into the Simmy folder and also link it somehow in the main readme and maybe put the Simmy logo as well in the main readme?

[martintmk]

@martincostello @martintmk I also think we should include some documentation in the readme as GitHub is the main point of contact so if there's nothing about Simmy in the repo's readme Simmy might be overlooked. We could create a readme file into the Simmy folder and also link it somehow in the main readme and maybe put the Simmy logo as well in the main readme?

Done:
#1912

[vany0114]

@martintmk where's the PR where you implemented the:

• AddChaosFault.AddException
• AddChaosResult.AddResult

[martintmk]

@vany0114

You can check the following PRs:

• Simmy API review Part 2 - Prefer Chaos over Monkey #1913 (by Peter)
• Introduce OutcomeGenerator #1095

[martintmk]

Hey @martincostello , @vany0114 , @peter-csala

Is there anything missing before we can release 8.3.0? From my perspective, the API looks great and there is nothing else to address. Maybe except:

Enabled's default value is false. I can guarantee that many people will 🤦🏻 when they realize that the missing piece is Enabled = true

We should change the default to true as per Peter's suggestion.

[peter-csala]

Hey @martincostello , @vany0114 , @peter-csala

Is there anything missing before we can release 8.3.0? From my perspective, the API looks great and there is nothing else to address. Maybe except:

Enabled's default value is false. I can guarantee that many people will 🤦🏻 when they realize that the missing piece is Enabled = true

We should change the default to true as per Peter's suggestion.

Yes, I'm working on that one. I'll raise a PR soon hopefully. Otherwise I have only one more question:

The BehaviorAction of the ChaosBehaviorStrategyOptions class seems a bit odd to me. Renaming it to Behavior or BehaviorGenerator feels more consistent with the other options.

[martintmk]

The BehaviorAction of the ChaosBehaviorStrategyOptions class seems a bit odd to me. Renaming it to Behavior or BehaviorGenerator feels more consistent with the other options.

Agree, let's use BehaviorGenerator to be consistent with other APIs.

[martincostello]

We were just waiting on documentation, but quite a few API changes have recently come up. We should be confident everything is definitely "right" before we ship 8.3, as then there's no going back with regards to breaking changes.

[martintmk]

We were just waiting on documentation, but quite a few API changes have recently come up. We should be confident everything is definitely "right" before we ship 8.3, as then there's no going back with regards to breaking changes.

I'll give the API a shot over the week and try to find some inconsistencies/improvements.

[peter-csala]

@vany0114 I still miss further reading links on the chaos engineering documentation page (something similar what we have on Rate limiter). Since the page talks mainly about Simmy (and not about chaos engineering in general) I think it might make sense to add links which are explaining the fundamentals (like hypothesis, steady-state, experiment, blast radius, etc.).

[martintmk]

@vany0114 I still miss further reading links on the chaos engineering documentation page (something similar what we have on Rate limiter). Since the page talks mainly about Simmy (and not about chaos engineering in general) I think it might make sense to add links which are explaining the fundamentals (like hypothesis, steady-state, experiment, blast radius, etc.).

Created related PR (#1937).

Regarding 8.3.0 release, we are good to go from my point of view. Both API and docs are great and we can still enhance those as we go (similar to what we did with Polly, where documentation is alive).

But the release depends on thumbs up from you folks. (@vany0114 , @peter-csala , @martincostello)

[vany0114]

Sorry for the absence guys, I've been enjoying some vacation but I'll be back on Monday, I'll review it, but I think it looks great and solid for the release, I agree with @martintmk

[peter-csala]

@martintmk Are we planning to release it as a separate package (Polly.Simmy) or as a part of the Polly.Core?

[martintmk]

@martintmk Are we planning to release it as a separate package (Polly.Simmy) or as a part of the Polly.Core?

It does not add any extra dependencies, so directly in Polly.Core.

[peter-csala]

It does not add any extra dependencies, so directly in Polly.Core.

It feels a bit odd to have chaos strategies inside the core package, but I can accept your reasoning.

[martintmk]

Hey folks, I was checking with .NET guys and I'll be also publishing blog post here:

https://devblogs.microsoft.com/dotnet/

Something along the lines of Resilience and chaos engineering with Polly, this should give 8.3.0 release some extra exposure.

[vany0114]

@martincostello @martintmk I also think we should include some documentation in the readme as GitHub is the main point of contact so if there's nothing about Simmy in the repo's readme Simmy might be overlooked. We could create a readme file into the Simmy folder and also link it somehow in the main readme and maybe put the Simmy logo as well in the main readme?

Guys I just reviewed the documentation and the latest changes on the APIs especially the ones on Fault and Result and it looks great, the docs and the examples look terrific thanks so much for the help with the docs!

The only thing that I do not 100% agree is the renaming changes: Monkey vs Chaos because of the reasons discussed here:

• API Review for Simmy in v8 #1571 (comment)
• API Review for Simmy in v8 #1571 (comment)

I wasn't aware you guys had made that decision already even though we had agreed previously to keep the Monkey naming as referenced above 🥺

So I think we should at least clarify that in the docs since as you can see in the below issue the users are expecting to find the MonkeyPolicies (MonkeyStrategies)

Polly-Contrib/Simmy#68

[martintmk]

@vany0114

So I think we should at least clarify that in the docs since as you can see in the below issue the users are expecting to find the MonkeyPolicies (MonkeyStrategies)

How about we add something along the lines of "Major differences" section to https://www.pollydocs.org/chaos ? (similar to https://www.pollydocs.org/migration-v8#major-differences)

[martintmk]

Hey folks, with #1951 done is there anything else preventing us releasing 8.3.0?

Fyi, my blog post is almost done, now I just need the new package that I can use and reference :)

[martincostello]

I'd just like some final confirmation from @vany0114 that everything is covered now.

Once everyone is happy, I can sort out pushing the package.

[vany0114]

I'd just like some final confirmation from @vany0114 that everything is covered now.

Once everyone is happy, I can sort out pushing the package.

@martincostello @martintmk I just left some minor feedback, otherwise I think everything is in place! Once the release is done I can pin an informational announcement in the old repo.

[martincostello]

Cool, if we think we're done then I'll close this issue.

Just need to merge #1954 then I can publish the release.

[martincostello]

The 8.3.0 packages are now all in NuGet.org.

[martintmk]

The blog post is up:

https://devblogs.microsoft.com/dotnet/resilience-and-chaos-engineering/

[martincostello]

Awesome - can you PR the link into the community links (whatever it's called) bit of the docs?