API Review for Simmy in v8 #1571

martintmk · 2023-09-08T05:50:52Z

Details on the issue fix or feature implementation

Based on #1459

Confirm the following

I started this PR by branching from the head of the default branch
I have targeted the PR to merge into the default branch
I have included unit tests for the issue/feature
I have successfully run a local build

martintmk · 2023-09-08T05:52:32Z

cc @martincostello, @joelhulen, @vany0114

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy/OutcomeChaosPipelineBuilderExtensions.cs

codecov · 2023-09-08T06:00:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (98f29c8) 84.53% compared to head (83f12df) 84.53%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1571   +/-   ##
=======================================
  Coverage   84.53%   84.53%           
=======================================
  Files         307      307           
  Lines        6777     6777           
  Branches     1043     1043           
=======================================
  Hits         5729     5729           
  Misses        839      839           
  Partials      209      209

Flag	Coverage Δ
linux	`84.53% <ø> (ø)`
macos	`84.53% <ø> (ø)`
windows	`84.53% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy.Outcomes/OutcomeStrategyOptions.TResult.cs

martintmk · 2023-09-08T06:20:30Z

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy/MonkeyStrategy.cs

+
+namespace Polly.Simmy;
+
+public abstract class MonkeyStrategy : ResilienceStrategy


Top Level Comment:
There are 3 terms in this PR that basically represent the same thing:

Monkey

Simmy

Chaos

Do you think we can consolidate and align this somehow?

@martintmk I think all of them are still valid tho, I mean from a brand standpoint if you will, back when Simmy was conceived, kinda the reasoning or the slogan was "Simmy is a monkey to create chaos" and it was developed mapping that, meaning that the Monkey is the one in charge to unleash the chaos as the more basic chaos unit let's say, thus the public base class that consumer can extend to implement their own monkies, for instance, I might want a monkey called Caesar that implements a strategy to stop nodes in a K8s cluster. Now, Simmy is our monkey implementation that offers a set of common chaos strategies.

@joelhulen thoughts?

Moved the this from the #1459 PR.

Alternative naming:

Polly.Simmy -> Polly.Chaos (namespace)

MonkeyStrategy -> ChaosStrategy

MonkeyStrategy<T> -> ChaosStrategy<T>

Personally, I feel this is a cleaner naming, but Simmy would lose the branding and effective dissolve into Polly.

@vany0114, @martincostello, @joelhulen

I have no strong thoughts on branded vs. not, but I agree that the naming should be consistent whichever is chosen.

My two cents is that Simmy has been around for a few years now, and there's fairly steady traffic to the repo (11 clones and 147 unique visitors in the past two weeks alone, not to mention almost 500 stars). I know that @vany0114 has done a bit of work raising awareness of the project. I also like the monkey logo :) All said, I think there is a bit of brand awareness that I don't think we necessarily need to throw away. However, I like the clarity of the proposed naming. But because of the previous points, I'm OK with sticking to the branded naming as long as, like @martincostello said, we stay consistent in the naming. We also should be very descriptive in the XML document and related published documentation to prevent any confusion.

If both @martincostello and @joelhulen are fine with current API and don't have any other suggestions I am fine exposing Simmy as it is now.

@vany0114 Can you create a tasks to expose Simmy in 8.2.0?

What needs to be done:

Make the API public

Prepare docs for new chaos strategies

Can you create a tasks to expose Simmy in 8.2.0?

@martintmk by "creating the tasks" you mean the PR(s) right?

Would be better to create an issue here:
https://github.com/App-vNext/Polly/issues/new/choose

Something like [Feature] Expose the Simmy APIs

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy/BehaviorChaosPipelineBuilderExtensions.cs

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy/EnabledGeneratorArguments.cs

martincostello · 2023-09-08T08:45:40Z

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy/LatencyChaosPipelineBuilderExtensions.cs

+
+namespace Polly.Simmy;
+
+public static class LatencyChaosPipelineBuilderExtensions


Similar thoughts on the class name here.

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy/OutcomeChaosPipelineBuilderExtensions.cs

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy/BehaviorChaosPipelineBuilderExtensions.cs

vany0114 · 2023-09-20T14:21:40Z

@martintmk I think this PR is not a draft anymore right? I guess that's why I cannot mark comments as resolved 🤔

martintmk · 2023-09-20T14:38:08Z

@martintmk I think this PR is not a draft anymore right? I guess that's why I cannot mark comments as resolved 🤔

Strange, maybe I should mark it as ready for review?

vany0114 · 2023-09-20T14:39:59Z

@martintmk I think this PR is not a draft anymore right? I guess that's why I cannot mark comments as resolved 🤔

Strange, maybe I should mark it as ready for review?

Yep I think so.

martintmk · 2023-11-14T15:40:54Z

API changes addressed and will be exposed in #1784

martintmk added the v8 Issues related to the new version 8 of the Polly library. label Sep 8, 2023

martintmk added this to the v8.0.0 milestone Sep 8, 2023

martintmk mentioned this pull request Sep 8, 2023

Once Simmy V8 is ready, we need to re-implement our FaultInjection component. dotnet/extensions#4306

Closed

martintmk commented Sep 8, 2023

View reviewed changes

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy/OutcomeChaosPipelineBuilderExtensions.cs Outdated Show resolved Hide resolved

martintmk commented Sep 8, 2023

View reviewed changes

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy.Outcomes/OutcomeStrategyOptions.TResult.cs Outdated Show resolved Hide resolved

martintmk commented Sep 8, 2023

View reviewed changes

martincostello reviewed Sep 8, 2023

View reviewed changes

martintmk commented Sep 8, 2023

View reviewed changes

ApiReview/API.Polly.Core/NoDocs/Polly.Simmy/BehaviorChaosPipelineBuilderExtensions.cs Outdated Show resolved Hide resolved

vany0114 mentioned this pull request Sep 18, 2023

Simmy v8 #1459

Merged

4 tasks

martintmk force-pushed the ApiReview branch from bf6e205 to 0c8fbec Compare September 20, 2023 06:11

martintmk mentioned this pull request Sep 20, 2023

Hide Simmy API for initial v8 release #1601

Merged

4 tasks

martintmk marked this pull request as ready for review September 20, 2023 17:00

martintmk removed the v8 Issues related to the new version 8 of the Polly library. label Sep 27, 2023

martintmk modified the milestones: v8.0.0, v8.1.0 Sep 27, 2023

vany0114 mentioned this pull request Oct 9, 2023

Simmy v8 feedback #1682

Merged

4 tasks

martintmk force-pushed the ApiReview branch from 0c8fbec to 27ba0cf Compare October 24, 2023 10:27

martincostello modified the milestones: v8.1.0, v8.2.0 Oct 30, 2023

martintmk added 2 commits November 1, 2023 13:43

API Review for Simmy

73047db

Update with latest API

2f08ea9

martintmk force-pushed the ApiReview branch from 27ba0cf to 2f08ea9 Compare November 1, 2023 12:43

Update with latest API

83f12df

martincostello modified the milestones: v8.2.0, v8.3.0 Nov 14, 2023

martintmk closed this Nov 14, 2023

martincostello deleted the ApiReview branch November 14, 2023 15:50

vany0114 mentioned this pull request Jan 31, 2024

Simmy API Review and Feedback #1901

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Review for Simmy in v8 #1571

API Review for Simmy in v8 #1571

martintmk commented Sep 8, 2023

martintmk commented Sep 8, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

martintmk Sep 8, 2023

martincostello Sep 8, 2023

joelhulen Oct 31, 2023

martintmk Nov 1, 2023

vany0114 Nov 1, 2023

martintmk Nov 1, 2023

martincostello Sep 8, 2023

vany0114 commented Sep 20, 2023

martintmk commented Sep 20, 2023

vany0114 commented Sep 20, 2023

martintmk commented Nov 14, 2023


		namespace Polly.Simmy;

		public abstract class MonkeyStrategy : ResilienceStrategy


		namespace Polly.Simmy;

		public static class LatencyChaosPipelineBuilderExtensions

API Review for Simmy in v8 #1571

API Review for Simmy in v8 #1571

Conversation

martintmk commented Sep 8, 2023

Details on the issue fix or feature implementation

Confirm the following

martintmk commented Sep 8, 2023

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

martintmk Sep 8, 2023

Choose a reason for hiding this comment

martincostello Sep 8, 2023

Choose a reason for hiding this comment

joelhulen Oct 31, 2023

Choose a reason for hiding this comment

martintmk Nov 1, 2023

Choose a reason for hiding this comment

vany0114 Nov 1, 2023

Choose a reason for hiding this comment

martintmk Nov 1, 2023

Choose a reason for hiding this comment

martincostello Sep 8, 2023

Choose a reason for hiding this comment

vany0114 commented Sep 20, 2023

martintmk commented Sep 20, 2023

vany0114 commented Sep 20, 2023

martintmk commented Nov 14, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading