New Syntax

[vany0114]

Hi @reisenberger,

This is the very beginning of the new syntax model for Simmy (WIP), just wanted to get feedback early with this PR.

Options objects

So, the idea is that every monkey policy has its own options objects, like this:

• Async options: InjectBehaviourAsyncOptions
• Sync options: InjectBehaviourOptions

Basic options

Applies for sync and async executions and the idea is to give to the user another overload to set a monkey policy easily using primitives rather than delegates. (when user doesn't care about cancellation, context, etc)

• DefaultOptions: Is a base class, every policy options should specialize it as need it, i.e: InjectBehaviourAsyncBasicOptions or InjectBehaviourBasicOptions

Syntax

Every monkey policy has its own option syntax, like this:

• Async options syntax: AsyncInjectBehaviourOptionsSyntax
• Sync options syntax: InjectBehaviourOptionsSyntax

Where every syntax offers two overloads, one with its own options objects and another one with the default ones. That way, I think, we're covering all the syntax that we're currently offering thru the old syntax.

Examples:

• First overload:

var policy = MonkeyPolicy.InjectBehaviourAsync(options =>
{
    options.InjectionRate = (context, ct) => GetInjectionRate(context, ct);
    options.Enabled = (context, ct) => GetEnabled(context, ct);
    options.Behaviour = (context, ct) =>
    {
         ...
    };
});

var policy1 = MonkeyPolicy.InjectBehaviourAsync(options =>
{
    options.InjectionRate = (_, __) => Task.FromResult(1.0);
    options.Enabled = (_, __) => Task.FromResult(true);
    options.Behaviour = (_, __)  =>
    {
          ...
    };
});

// etc...

• Second overload:

var policy = MonkeyPolicy.InjectBehaviour(options =>
{
    options.InjectionRate = 0.6;
    options.Enabled = () => false;
    options.Behaviour = () => { injectedBehaviourExecuted = true; };
});

Let me know your thoughts, please.

Note: I think after this change, we should mark as obsolete the old syntax overloads, right?

[vany0114]

I was left wondering, and I think that we need to change the constructor's signature of monkey policies (latency, fault, etc) as well (or add another overload to continue supporting the old syntax as is), to inject the options object rather than the delegates separately. For example with the IsCancellable case on latency policy, w/o the options object, we would need to add another parameter to the policy itself...over and over again every time we need a new parameter in a policy.

Like this:

// current ctor (we can keep it to continue supporting the old syntax.)
internal AsyncInjectBehaviourPolicy(Func<Context, CancellationToken, Task> behaviour, Func<Context, CancellationToken, Task<Double>> injectionRate, Func<Context, CancellationToken, Task<bool>> enabled)
    : base(injectionRate, enabled)
{
    _behaviour = behaviour ?? throw new ArgumentNullException(nameof(behaviour));
}

// new ctor to support the new syntax
internal AsyncInjectBehaviourPolicy(InjectBehaviourAsyncOptions options)
    : base(options.InjectionRate, options.Enabled)
{
    _behaviour = options.Behaviour ?? throw new ArgumentNullException(nameof(options.Behaviour));
}

[reisenberger]

Hey @vany0114 . Thank you so much for getting us into action with this syntax! It's great to be doing this for Simmy, also because we are hopefully thinking to do similar with Polly. Thank you also for the super clear questions/explanations - very helpful.

I love the new InjectBehaviourAsync(Action<InjectBehaviourAsyncOptions> configureOptions) overload.

I had an idea that maybe we could use method-chaining, to make a builder syntax, and separate more the individual items to be configured. To explore this, it seemed easier to make an example than give long-description, so I made a sketch in this branch. It builds on your commit (because of the great stuff already in there), so it is also easy to see the differences in the commit.

What do you think?

Configuring a policy ends up looking something like this: For a simple case (more constants than delegates):

var policy = MonkeyPolicy.InjectBehaviourAsync(with =>
    with.Behaviour(() =>
        {
            injectedBehaviourExecuted = true;
            return Task.CompletedTask;
        })
        .InjectionRate(0.6)
        .Enabled(false)
    );

For a more complex case (delegates):

var policy = MonkeyPolicy.InjectBehaviourAsync(with =>
    with.Behaviour((context, ct) => { ... })
        .InjectionRate((context, ct) => GetInjectionRate(context, ct)) // or just .InjectionRate(GetInjectionRate) 
        .EnabledWhen((context, ct) => GetEnabled(context, ct)) // or just .EnabledWhen(GetEnabled)
    );

Or mixing complex and simple:

var policy = MonkeyPolicy.InjectBehaviourAsync(with =>
    with.Behaviour(() => { ... })
        .InjectionRate((context, ct) => GetInjectionRate(context, ct))
        .Enabled()
    );

Thoughts about this sketch?

[reisenberger]

One observation/question about the sketch ^: I made all the properties on the XOptions classes internal, to avoid having a mixture of property-setting and setting-via-method-chaining.

For Simmy so far, it looks like that would probably make sense. But it doesn't have to be this way. Theoretically, if a policy has some properties which are constant primitives (eg number of retries; duration of circuit-breaking), it would be possible to leave those with public setters (so that they could be eg loaded with standard .NET Core Options/Configuration patterns) (x-ref App-vNext/Polly#143), with the other properties internal and settable by method-chaining.

I do not have a strong view at this point whether such a mixture would be a good idea; it might lead to overloads like:

var policy = Policy
    .Handle<FooException>()
    .CircuitBreaker(circuitBreakerOptions, with =>
        with.OnBreak(() => { ... })
    );

// Or maybe:
var policy = Policy
    .Handle<FooException>()
    .CircuitBreaker(configure =>
        configure.FromOptions(circuitBreakerOptions)
            .OnBreak(() => { ... })
    );

Maybe this mixture would be more a question for later; but mentioning it because it was an intentional change in the code sketch to make the properties internal.

[reisenberger]

@vany0114 Re this comment: Yes, agree with all! 👍

inject the options object rather than the delegates separately
internal AsyncInjectBehaviourPolicy(InjectBehaviourAsyncOptions options)

yes, much better.

Re:

[after changes], we should mark as obsolete the old syntax overloads, right?
// current ctor (we can keep it to continue supporting the old syntax.)
internal AsyncInjectBehaviourPolicy(Func<Context, CancellationToken, Task> behaviour, // etc

Yes, we could mark it [Obsolete] as soon as a new syntax is introduced. And we don't have to extend it each time a new option is added to a Simmy policy; it should be frozen if we don't intend to support it long-term. In fact, because Simmy is quite young (and the release number is 0.X, not 1.X), we could probably remove the old syntax quite quickly (if we wanted to - open to your views!)

---

(For Polly we would have to think more carefully about supporting old syntax (though we would still freeze it). Maybe we would use a 'compatibility pack' - I have some ideas.)

[vany0114]

Hey @reisenberger. I really like how the syntax looks using method-chaining, I think is easier to use not to mention it's more intuitive. Thanks for bringing such a cool idea.

Regarding mixing property-setting and setting-via-method-chaining, yeah I think we can go first only with setting-via-method-chaining to keep it simple and see whether later on the community ask for property-setting feature (I think they will because of you say about loading with standard .NET Core Options/Configuration patterns)

And I agree that we can remove the old syntax since as you said, Simmy is still pretty young and we're not even in a 1.X version.

Thanks again for taking the time reviewing this PR and for the cool ideas you suggest (as usual)!
I'm gonna resume this PR to try to release the new syntax in the next couple of weeks, hopefully.

[reisenberger]

Sounds good @vany0114 ! Thanks for so taking ownership of this and pushing Simmy along so awesomely! Please let me know when/if I can be useful to review a next stage together (but sounds like we are on a great path!). (I didn't look at latest commit yet - just shout when you want me to circle back round and look)

Again: thanks! 💯

[vany0114]

@reisenberger I think I've finished with this PR, it's a huge one, but all the work comes down to this:

• New monkey Options classes (sync/async)
• New monkey Options extensions classes (sync/async)
• New syntax classes based on Options(sync/async)
• Updates old syntax classes to make them obsolete
• Updates policies to add new constructor using options class
• Updates old unit tests to make them obsolete
• New unit tests for the new syntax

Also, I took advantage to solve #15. I've created this guy to ensure the injection rate not only from the engine at execution time but also at configuration time, and I added some unit tests around that.

Regarding the old syntax, I ended up keeping it just to be kinda kind with the users 😅 I mean, I think it's nice to warn them about that first, before to break their code, but I think for version 1.0 (hopefully we'll be able to make it this year) we can get rid of all the deprecated code, what do you think?

Thanks in advance for your time reviewing PR's 👀 and also to bringing cool ideas, I really appreciate it!

---

Note: Once we finish with this PR, I'll create another one to update the documentation.

[vany0114]

I think when we release this enhancement, it's going to be easier to address the #16, #17 and #18 issues 😃 ...actually I'm gonna create the issue to support the IsCancellable property from Latency monkey.

[vany0114]

@reisenberger BTW I've created this variation (actually just using a different name) into the FaultBuilder extensions

Basically to do this:

var policy = MonkeyPolicy.InjectFault<ResultPrimitive>(with =>
  with.Result(fault) // instead of: with.Fault(...)
    .InjectionRate(0.4)
    .Enabled()
);

Not sure about that, I was thinking it might be more descriptive for the user, what do you think?

[reisenberger]

@vany0114 Looks great to me! 👍 💯

Two small things we could clean-up (if you agree?):

• Shall we lose the Options subfolders and subnamespace? IE move everything in those folders up one level. It would be simpler for users to only have one using statement, eg using Polly.Contrib.Simmy.Latency;, not also using Polly.Contrib.Simmy.Latency.Options;

• Shorten configureOptions.Invoke(options); to configureOptions(options); ?

(I think these were both things in the earlier sketch, sorry for raising them now!)

[reisenberger]

@vany0114 Re your great question around "inject result" language instead of "inject fault":

great question, because maybe the user would like sometimes to inject something which isn't strictly a fault, maybe they want to inject a 301 redirect (or a 200OK even, in a unit test). So maybe our API language should be more neutral than "fault"? (but offer a wording with "fault", for the common case).

I was also wondering about the near-collision between:

• .InjectFault(Action<InjectFaultOptions<Exception>> configureOptions)
• .InjectFault<TResult>(Action<InjectFaultOptions<TResult>> configureOptions).

There is enough difference that the compiler can distinguish, but it could still be confusing for users. (I could see a user writing .InjectFault<HttpRequestException>(...) and wondering why it didn't work.) We could take this opportunity to distinguish them, by changing the API wording to:

• .InjectException(Action<InjectFaultOptions<Exception>> configureOptions)
• .InjectResult<TResult>(Action<InjectFaultOptions<TResult>> configureOptions).

InjectFaultOptionsExtensions can then have config overloads for both .Fault(...) and .Result(...), as you already suggest (syntactic sugar only; both do the same thing, right?) . So users could code:

var policy = MonkeyPolicy.InjectResult<ResultPrimitive>(with =>
  with.Fault(fault)
    .InjectionRate(0.4)
    .Enabled()
);

as well as:

var policy = MonkeyPolicy.InjectResult<ResultPrimitive>(with =>
  with.Result(somethingNotNecessarilyAFault) // instead of: with.Fault(...)
    .InjectionRate(0.4)
    .Enabled()
);

What do you think? Or any other suggestion?

Although this ^ is an API change (InjectFault -> InjectResult), now is the time to do it, we are only at 0.X semver, so it is understood we can change this now to get it right before 1.0

[vany0114]

@vany0114 Looks great to me! 👍 💯

Two small things we could clean-up (if you agree?):

• Shall we lose the Options subfolders and subnamespace? IE move everything in those folders up one level. It would be simpler for users to only have one using statement, eg using Polly.Contrib.Simmy.Latency;, not also using Polly.Contrib.Simmy.Latency.Options;
• Shorten configureOptions.Invoke(options); to configureOptions(options); ?

(I think these were both things in the earlier sketch, sorry for raising them now!)

Yeah, agree with both of them. You're right, the fact to have an extra using might be annoying, good catch!

[reisenberger]

You're right the fact to have an extra using might be annoying, good catch!

No problem, it was my bad originally! I checked various Microsoft repos and it's certainly standard not to have an .Options sub-namespace

[vany0114]

@reisenberger Also agree with changing the API wording. I'll work thru that. And thanks for taking the time to review it in such a thorough way! 👍 🥇

[vany0114]

Shall we lose the Options subfolders and subnamespace? IE move everything in those folders up one level. It would be simpler for users to only have one using statement, eg using Polly.Contrib.Simmy.Latency;, not also using Polly.Contrib.Simmy.Latency.Options;

@reisenberger what do you think if we keep the Options folders, just for line up the options stuff and keep them separate from the policy and syntax, at least visually is easier to find and distinguish them, and we just remove the sub-namespace?

[reisenberger]

what do you think if we keep the Options folders, just for line up [...] and we just remove the sub-namespace?

My suggestion would be removing the Options folder as well; it's a common expectation for repos that namespaces will correspond with folder locations of classes (but I don't feel strongly if you want to do otherwise).

For background: With Polly, we had feedback about that being unexpected for the static-method Policy syntax, ie the fact that classes like this live in folder Polly/PolicyType (or here Polly.Contrib.Simmy/PolicyType) whereas in fact the namespace is Polly.Contrib.Simmy and they are partial parts of a class living in that root folder/namespace.

I had done that because I wanted to keep everything about one policy together (in folder ./PolicyType), but an alternative could be to move those partial syntax classes down to the / root namespace of the project, as eg:

• MonkeyPolicy.BehaviourSyntax.cs
• MonkeyPolicy.LatencySyntax.cs

(We don't necessarily have to solve the decision about location of syntax partial classes of MonkeyPolicy as part of this PR, so we can get your main syntax changes out 👍 - but interested in your views.)

[vany0114]

I thought we could keep the Options folder because as you said PolicyType folders have the same root namespace, but yeah the normal pattern is the namespaces correspond with folder locations. So thanks for sharing the Polly feedback because is great to learn from community feedback.

Said that, I'll remove the Options folder as you suggested, and I think we can keep the PolicyType folders because I feel the same, it's better to keep everything about one policy together.

[vany0114]

Hi @reisenberger I just finished with the suggestions, I also took advantage to implement #16, I just renamed the Fault namespaces to be Outcomes and the API wording as InjectException and InjectResult .

[reisenberger]

@vany0114 This is great!

---

The only thing I spotted: I had thought that we should move the XOptions classes up only one level, so that they still were in the Polly.Contrib.Simmy.<PolicyType> namespace.

I experimented with doing that (to see if it caused any other problems), and noticed that, if a consuming class (a test class in our case) didn't have the appropriate using Polly.Contrib.Simmy.<PolicyType>; statement, the IDE intellisense would give an error (eg):

Could not access internal property 'Behaviour' here

For example, this occurred where InjectBehaviourWithOptionsSpecs wanted to access the method public static InjectBehaviourOptions Behaviour(this InjectBehaviourOptions options, Action behaviour) from InjectBehaviourOptionsExtensions. It conflicted with the property internal Action<Context, CancellationToken> Behaviour

This is not very satisfactory, because it risks letting users fall into a pit-of-failure (not pit-of-success), because it is quite likely that users copy code-snippets into their code without copying the accompanying using statements. This is my bad. It looks like the original sketch was 'too clever' (has this risk) if naming the internal property and the extension method the same thing.

So TL;DR I made a commit to rename relevant internal properties to XInternal.

Then I noticed some classes/filenames where we still had 'Fault' and could say 'Outcome'; I pushed a commit to rename those.

---

Apologies for jumping in with commits, it looked as if some of my previous suggestions might have caused some problems, so I cleaned up after myself this time 😉

What do you think? If you are happy with this branch, we could PR that onto your PR here, and then merge this #49? Or can you spot anything else?

It's great that we have done all the work on this, because it's a great model for Polly too! 👍

[vany0114]

@reisenberger those are really good catches, thanks for fixing them!

Then I noticed some classes/filenames where we still had 'Fault' and could say 'Outcome'; [...]

Apologies for that, I thought I had done with it but I was far away 😅

What do you think? If you are happy with this branch, we could PR that onto your PR here, and then merge this #49? Or can you spot anything else?

Yeah, totally agree, let's do it!

[reisenberger]

Maybe when we finally merge this to v0_2_1 (after #55) we can squash-merge because of the many steps/clean-ups on the way 🙂

[vany0114]

Maybe when we finally merge this to v0_2_1 (after #55) we can squash-merge because of the many steps/clean-ups on the way 🙂

Yeah, definitely. Also, I think I have to rename de branch, at the beginning I thought it was going to be just for #48 and maybe for #46 that's why I named v0_2_1.

What do you think?

[reisenberger]

Also, I think I have to rename de branch

Yes, feel free to rename it if you want! (Though after it is merged to master and the release is made with the correct release number, maybe it doesn't matter so much ...)