Outcome chaos strategy

[peter-csala]

I was preparing some sample code for the outcome chaos strategy cheat sheet.
I'm bumped into the following inconsistencies:

1. The namespace is in plural

• Polly.Simmy.Behavior
• Polly.Simmy.Fault
• Polly.Simmy.Latency
• Polly.Simmy.Outcomes

Note

Am I right that we can't really do anything without introducing a breaking change?

2. FaultGenerator vs OutcomeGenerator

It is required but can be null

[Required]
public Func<FaultGeneratorArguments, ValueTask<Exception?>>? FaultGenerator { get; set; }

It is required and can't be null

[Required]
public Func<OutcomeGeneratorArguments, ValueTask<Outcome<TResult>?>> OutcomeGenerator { get; set; }

Note

Can we fix FaultGenerator by not allowing null?

3. Outcome.FromResultAsValueTask

Due to the fact that the return type is defined as ValueTask<Outcome<TResult>?>
creating a synchronous result is really clumsy

ValueTask.FromResult<Outcome<HttpResponseMessage>?>(Outcome.FromResult(new HttpResponseMessage(HttpStatusCode.InternalServerError)))

if it could be ValueTask<Outcome<TResult>>? similar to Hedging's ActionGenerator

Polly/src/Polly.Core/Hedging/HedgingStrategyOptions.TResult.cs

Line 55 in 3e2164e

public Func<HedgingActionGeneratorArguments<TResult>, Func<ValueTask<Outcome<TResult>>>?> ActionGenerator { get; set; } = args =>

then people could use Outcome.FromResultAsValueTask:

Outcome.FromResultAsValueTask<HttpResponseMessage?>(new HttpResponseMessage(HttpStatusCode.InternalServerError))

Note

Can we fix the return type of OutcomeGenerator?

4. Outcome with exception

I think it might make sense to emphasize in the documentation that Outcome chaos strategy should NOT be used to inject exception. IMHO adding an anti-pattern by using the Outcome.FromException could be helpful.

Also I think it would make sense to add some sample code where we use both Fault and Outcome strategies to show how to use them as intended.

Note

Do you guys agree?

cc: @martincostello , @martintmk , @vany0114

[martintmk]

1. Polly.Simmy.Outcomes

Yeah, at this point we need to leave it as it is. Not a big deal though.

2. FaultGenerator vs OutcomeGenerator

let's make OutcomeGenerator nullable and required as we cannot assign it any sensible default value. Similar to FaultGenerator .

3. Outcome.FromResultAsValueTask

I agree, it's clumsy :/. The hedging generator is slightly different, it's not the task that is nullable, but the action generator: Func<ValueTask<Outcome>>?

An even worse, I think we have a bug here, if null Outcome is returned we will get an exception:

Polly/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs

Line 28 in 3e2164e

var args = new OnOutcomeInjectedArguments<T>(context, outcome.Value);

I would say let's close our eyes for a second make sure no-one is watching and do a breaking change. It's only nullable annotation though:

public Func<OutcomeGeneratorArguments, ValueTask<Outcome<TResult>>> OutcomeGenerator { get; set; }

ValueTask<Outcome<TResult>?> -> ValueTask<Outcome<TResult>>

@martincostello Thoughts?

4. Outcome with exception

I think it might make sense to emphasize in the documentation that Outcome chaos strategy should NOT be used to inject exception.

This is true when someone uses chaos outcome strategy ONLY to inject exceptions. In that case the fault chaos should be used. However, using chaos outcome to inject both results and exceptions is IMHO a valid pattern.

[vany0114]

An even worse, I think we have a bug here, if null Outcome is returned we will get an exception:

How is that a bug? To me null is a valid outcome. There's a unit test that covers that scenario.

Polly/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs

Line 136 in 3e2164e

public async Task Given_enabled_and_randomly_within_threshold_should_inject_result_even_as_null()

[martintmk]

It works because the test is not using null instance of outcome as the nullability contract allows:

Polly/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs

Line 138 in 3e2164e

Outcome<HttpStatusCode?>? nullOutcome = Outcome.FromResult<HttpStatusCode?>(null);

If you changed that to:

Outcome<HttpStatusCode?>? nullOutcome = null;

It would fail. I would still prefer changing the nullable annotations as the usage becomes much simper and the user can use the Outcome factories.

#1982

The nullability annotations do not affect runtime, otherwise I would not consider this change.

[vany0114]

Oh I see and to be clear, with that change the consumer will be able to inject a null outcome then? that is a super common scenario.

[martintmk]

They would be able to inject null, but the outcome strategy would always expect to get an outcome instance. The intention with current API was that returning null Outcome would result in chaos strategy no injecting anything and invoking the callback.

[vany0114]

Yeah I mean, we need to be able to provide the consumer a way to inject a null outcome bc null does not mean the strategy should not inject anything and just invoke the callback (that's the bug that you're fixing I guess).

SomeObject foo = await GetFoo(); // they might need to simulate when the GetFoo method returns null
if (foo == null) {
  DoSomething();
}

...

[vany0114]

Outcome with exception

I agree with @martintmk that using the outcome chaos strategy to inject exceptions does not feel wrong, however, I also think that we should be clear in the docs and explain the "caveats/implications" of doing so, for example, metrics are one of them, for instrumentation purposes it's way clearer looking for fault injected events to be 100% sure where/when exceptions were injected, not to mention that when using the OnOutcomeInjected delegate the consumer might need to check if the outcome was an exception, etc.

@peter-csala So in that sense, IMHO it could become an anti-pattern, so yeah definitively it is worth explaining that in the docs.

[peter-csala]

Just to summarize:

1. It remains as it is
2. OutcomeGenerator's signature will be updated inside Nullability fixes for chaos outcome strategy #1982 by @martintmk
3. ☝🏻
4. Either @vany0114 or me will create a PR for that. @vany0114 do you want to do it?

[vany0114]

Yep I can help with that

[vany0114]

PR is open for review: #1994