Document outcome strategy anti-patterns

[vany0114]

Pull Request

The issue or feature being addressed

• Document outcome strategy anti-patterns
• Deprecates AddException API in the OutcomeGenerator

Details on the issue fix or feature implementation

As per our discussion here we decided to document why injecting exceptions via the outcome strategy is an anti-pattern (in advanced scenarios when the usage of delegates is required):

• side effects on telemetry events, which might affect instrumentation and metrics
• goes against controlled chaos injection

As part of this change, I marked the AddException API in the OutcomeGenerator as deprecated to be consistent with what we're preaching, it does not make sense to say it is an anti-pattern and at the same time provide a built-in way to do so + if we expose an API to inject an exception as an outcome why have the fault strategy at all? why consumers would use it? or when to use one or another? it ends up creating confusion.

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.67%. Comparing base (68a3c7f) to head (5a7c5e4).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1994      +/-   ##
==========================================
+ Coverage   83.64%   83.67%   +0.02%     
==========================================
  Files         312      312              
  Lines        7105     7105              
  Branches     1054     1054              
==========================================
+ Hits         5943     5945       +2     
  Misses        789      789              
+ Partials      373      371       -2     

Flag	Coverage Δ
linux	?
macos	?
windows	83.67% <ø> (+0.02%)	⬆️

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.

[vany0114]

Hi @martincostello @peter-csala I just addressed the rest of the feedback, please take a look again when you guys have the chance.

[peter-csala]

The usage samples advocate to use one of the anti-patterns

...
.AddResult(context => new HttpResponseMessage(CreateResultFromContext(context))) // Access the ResilienceContext to create result
.AddException<HttpRequestException>(), // You can also register exceptions

Please update these OutcomeGenerators by removing theAddException clauses.

[vany0114]

When I originally opened the PR I had removed those guys since I had deprecated it, however, I put them back so that we support that scenario (injecting results and exceptions in the same strategy) but under their judgment after knowing the caveats/side effects explained in the anti-pattern sections and in the footer. I can remove that entirely from the docs but it would be hidden. Honestly, while I understand it is handy it feels like we're kinda discouraging people to use the right strategy to inject faults.

We could also add a comment in the samples as I did in some snippets, like this: // ⚠️ Avoid this ⚠️
and maybe add a remarks on the API pointing to the docs? thoughts?

[vany0114]

@martincostello I think this PR is ready to go BTW.

[martincostello]

Yep, I'm just AFK a lot at the moment and haven't had the chance to re-review properly. It's in my todo queue.