Add Outcome to the BreakDurationGeneratorArguments

[peter-csala]

Pull Request

The issue or feature being addressed

• Currently the Circuit Breaker could not take advantage of the RetryAfter header to dynamically set its break duration

Details on the issue fix or feature implementation

• Extended the BreakDurationGeneratorArguments with Outcome

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

[martincostello]

New entries should go in the Unshipped file. What you're doing here is effectively "tricking" the Public API analyser into thinking breaking changes aren't.

[peter-csala]

Well, that's quite unfortunate. :(

That would require me to create a BreakDurationGeneratorArguments.TResult.cs. Which is fine but inside the CircuitBreakerStrategyOptions.TResult.cs I have to create a new generator to make this change backward compatible.

 public Func<BreakDurationGeneratorArguments, ValueTask<TimeSpan>>? BreakDurationGenerator { get; set; }

 public Func<BreakDurationGeneratorArguments<TResult>, ValueTask<TimeSpan>>? NewBreakDurationGenerator { get; set; }

Do you have any idea how to solve this problem?

[martincostello]

Not easily, no - is what you're trying to do achievable via storing the result in the context?

[peter-csala]

The OnOpened is called after the break duration calculation, because the OnCircuitOpenedArguments contains the break duration as well.

So, I can't set the context from the OnOpened that's why I have to set it inside the user callback. Which is not possible if I decorate my HttpClient via the AddResilienceHandler. :(

[martincostello]

Maybe @martintmk has some ideas.

If the type was a class we could just inherit from it and add a generic version, the we wouldn't need to touch the generator but you'd need to do a cast /as to get the generic version. As it's a struct we can't do that.

I don't think introducing a "NewBreakDurationGenerator" parallel dimension would be a good move and I don't think we'd want to go down the route of a v9 to make this kind of breaking change any time soon.

[peter-csala]

Yeah, this NewBreakDurationGenerator idea is non-sense. I've just used it to illustrate the problem.

If my memory serves well this is the 3rd or 4th time when we do not fix something on the API due to backward compatibility. Would it make sense to collect these under a ticket and label it with v9?

[martincostello]

We could maybe use the existing breaking change label and park an issues that would be nice to have but would be breaking there.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.87%. Comparing base (1eb48d7) to head (3bdcafb).
Report is 142 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2200      +/-   ##
==========================================
- Coverage   83.88%   83.87%   -0.01%     
==========================================
  Files         313      313              
  Lines        7172     7171       -1     
  Branches     1060     1060              
==========================================
- Hits         6016     6015       -1     
  Misses        785      785              
  Partials      371      371              

Flag	Coverage Δ
linux	83.87% <100.00%> (-0.01%)	⬇️
macos	83.87% <100.00%> (-0.01%)	⬇️
windows	83.87% <100.00%> (-0.01%)	⬇️

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.

[martintmk]

I am thinking, how about we add a Outcome<object> Outcome field to existing BreakDurationGeneratorArguments?

[peter-csala]

I am thinking, how about we add a Outcome<object> Outcome field to existing BreakDurationGeneratorArguments?

That could solve the breaking change problem.

But the usage of the Outcome would be inconsistent with the rest of the argument structures/classes.

[github-actions]

This pull request is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further changes are made.

[github-actions]

This pull request is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further changes are made.