Refactor/Improve Polly's resilient code

[CESARDELATORRE]

Feedback provided by Dylan Reisenberge, from Polly's organization.
We'll refactor this when possible.
Only number 1 is really desirable to change (when want to make shutdown graceful). The rest are good to have.

1. Consider Polly's Execute/ExecuteAsync() overloads taking CancellationToken to facilitate graceful shutdown.
2. Option to use Polly's FallbackPolicy in place of catch(BrokenCircuitException) { / fallback / }
3. Option to use Polly's HandleResult(Func<T, bool>) to .Handle(r => r.StatusCode == StatusCode.InternalServerError) .
4. Option to use Polly's context.ExecutionKey and context.PolicyKey to capture richer execution context in logging.

DETAIL:
1. WORTH CONSIDERING Graceful shutdown by CancellationToken: At the moment, all Polly usages are Execute/ExecuteAsync() overloads not taking a CancellationToken. Using instead Execute/Async() overloads taking a CancellationToken might be useful when implementing graceful shutdown. Using the Execute(, CancellationToken) overloads, any requests in the 2/4/8/16/32-second wait parts of exponential backoff will exit as soon as the CancellationToken is signalled (throwing usual OCE). Our microservice projects use this: a central CancellationToken is signalled for graceful shutdown, and all pending requests/operations cancel promptly (and can log they canceled etc). The CancellationToken passed to Polly's Execute/Async(...) can also be passed on to the delegate you execute, eg: policy.ExecuteAsync(ct => apiClient.PutAsync(/* other params*/, ct), someCancellationToken);

2. (OPTION) FallbackPolicy: Where the code has catch(BrokenCircuitException) in the controller (eg

eShopOnContainers/src/Web/WebMVC/Controllers/OrderController.cs

Lines 52 to 55 in 0c317d5

	catch(BrokenCircuitException)
	{
	ModelState.AddModelError("Error", "It was not possible to create a new order, please try later on");
	}

): you have also the option of Polly's in-built Fallback Policy (https://github.com/App-vNext/Polly/wiki/Fallback). The concept is like:

Policy.Handle<BrokenCircuitException>().FallBack(() => ModelState.AddModelError("Error", "It was not possible to create a new order, please try later on")); // executes the Fallback action if the policy catches the exception. 

and you wrap that FallbackPolicy outermost in the PolicyWrap. You could take the Polly dependency entirely out of OrderController, and here (

eShopOnContainers/src/Web/WebMVC/Controllers/OrderController.cs

Line 46 in 0c317d5

await _orderSvc.CreateOrder(model);

) do something like:
await _orderSvc.CreateOrder(model, onError: () => ModelState.AddModelError("Error", "It was not possible to create a new order, please try later on"));
and pass the Fallback Action onError down to ResilientHttpClient. Not saying this is is preferable. Advantages and disadvantages: adds the fallback Action parameter to the chain of calls; and would change of signatures of your IHttpClient methods, to differ from System.Net.HttpClient.

3. (OPTION) HandleResult(...): Where the code throws on status code 500 to help the circuit-breaker (

eShopOnContainers/src/BuildingBlocks/Resilience/Resilience.Http/ResilientHttpClient.cs

Lines 44 to 49 in 0c317d5

	// raise exception if HttpResponseCode 500
	// needed for circuit breaker to track fails
	if (response.Result.StatusCode == HttpStatusCode.InternalServerError)
	{
	throw new HttpRequestException();
	}

), you have the option instead of using Polly's HandleResult(Func<T, bool>) or OrResult<>(). You can construct a Policy like:
Policy
.Handle()
.OrResult(r => r.StatusCode == StatusCode.InternalServerError)
.WaitAndRetryAsync( /* as before */ )
An implication: To use .HandleResult(Func<T, bool>), your Policy becomes strongly-typed Policy. So the implementation of ResilientClient.GetStringAsync(...) (

eShopOnContainers/src/BuildingBlocks/Resilience/Resilience.Http/ResilientHttpClient.cs

Line 34 in 0c317d5

public Task<string> GetStringAsync(string uri) =>

) would have to be changed internally to use one of the System.Net.HttpClient.GetXAsync(...) overloads returning Task. Again, advantages and disadvantages.
4. (TIDY UP) Logging from Polly's execution context: Here (https://github.com/dotnet-architecture/eShopOnContainers/blob/0c317d56f3c8937f6823cf1b45f5683397274815/src/Web/WebMVC/Infrastructure/ResilientHttpClientFactory.cs#L36-l37) the code logs from Polly's context.ExecutionKey and context.PolicyKey. At the moment the code isn't setting them to anything. These properties are great though for making logging capture particulars of the call context, when you are using policies centrally like the code does in ResilientHttpClient and EventBusRabbitMQ . ResilientHttpClient and EventBusRabbitMQ () (and similars) could set context.PolicyKey on the policies. context.ExecutionKey could be set (by something higher up the call chain) to indicate which operation is involved (eg GetOrder, GetMyOrders, CreateOrder), either passed in or set on some context flowing with the async calls. Reference: https://github.com/App-vNext/Polly/wiki/Keys-and-Context-Data . Or remove references to context.ExecutionKey and context.PolicyKey if not setting them.

[reisenberger]

👍 @CESARDELATORRE . Come back to me if anyone wants any elaboration, nearer the time.

[reisenberger]

Re 4 above: If/when the new form of ResilientHttpClient in dev branch is merged, then this line:

return policyWrapper.ExecuteAsync(() => action());

can be changed very simply to set Polly's ExecutionKey for the execution, based on the origin variable where you capture origin of the call, thus:

return policyWrapper.ExecuteAsync(() => action(), new Context(origin));

Then your rich logging here will reflect that origin of the call, for everything the Polly policies log.

[mvelosop]

Closed as it was included in the Roadmap (vNext).