[API Proposal]: Allow retrieval of HttpRequestMessage from Polly.Context

[martintmk]

Background and motivation

Clients using new resilience APIs based on top of Polly v8 might want to access HttpRequestMessage associated with the attempt being executed. This would allow the following scenarios:

Metering Enrichment

Client can decide to add additional metrics extracted from HttpRequestMessage using metering enrichment.

Custom Routing

For hedging or retries, the client might decide to change the URL for secondary attempts. This would also enable dynamic URL resolution.

Access to HttpRequestMessage in callbacks

For logging/other purposes the client uses callbacks and can be interested in the current request message.

API Proposal

namespace Polly;

public static class HttpResilienceContextExtensions
{
    public static HttpRequestMessage? GetRequestMessage(this ResilienceContext context);
    public static void SetRequestMessage(this ResilienceContext context, HttpRequestMessage? requestMessage);
}

API Usage

services
    .AddHttpClient("my-client", client => client.BaseAddress = new Uri("https://primary-endpoint"))
    .AddStandardHedgingHandler()
    .Configure(options =>
    {
        options.Hedging.OnHedging = args =>
        {
            // Retrieve the request message
            HttpRequestMessage request = args.ActionContext.GetRequestMessage();

            UriBuilder uriBuilder = new(request.RequestUri);
            uriBuilder.Host = "secondary-endpoint";

            // Override the request URI
            request.RequestUri = uriBuilder.Uri;

            return default;
        };
    });

The example above demonstrates how the new API can be used to change the URL of outgoing request. Similar approach can be used for retries or telemetry enrichment.

Alternative Designs

The above example can be rewritten as:

ResiliencePropertyKey<HttpRequestMessage> requestMessageKey = new("Resilience.Http.RequestMessage");

services
    .AddHttpClient("my-client", client => client.BaseAddress = new Uri("https://primary-endpoint"))
    .AddStandardHedgingHandler()
    .Configure(options =>
    {
        options.Hedging.OnHedging = args =>
        {
            // Retrieve the request message
            HttpRequestMessage? request = args.ActionContext.Properties.GetValue(requestMessageKey, null!);

            UriBuilder uriBuilder = new(request.RequestUri);
            uriBuilder.Host = "secondary-endpoint";

            // Override the request URI
            request.RequestUri = uriBuilder.Uri;

            return default;
        };
    });

However, this relies on some internal naming:
https://github.com/dotnet/extensions/blob/a8e17517bd5150cc0f992b66aa6591c7c6fbdfc4/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/ResilienceKeys.cs#L13C92-L13C122

It would be better to have built-in support for retrieving HttpRequestMessage from ResilienceContext as I suspect this will be a common scenario.

Risks

Caller retrieving and modifying HttpRequestMessage associated with the current attempt in a non-compatible way or doing some changes that will corrupt the request.

[joperezr]

Given you are the main expert on this, are you expecting any feedback from anyone else @martintmk or is this ready for review?

[martintmk]

Given you are the main expert on this, are you expecting any feedback from anyone else @martintmk or is this ready for review?

I believe this is ready for review.

[davemyers-dev]

@martintmk I'd love to see this built in! We're trying to rollout new v8 policy guidance & extensions for our enterprise and using information from the request message in our Retry logging is a common use case.

[iliar-turdushev]

@martintmk I have 2 questions:

• As I understand, SetRequestMessage will be useful for the Hedging handler when a user wants to replace the request object with a custom one, right?
• Do we want to make the second argument of the SetRequestMessage method non-nullable, i.e. SetRequestMessage(this ResilienceContext context, HttpRequestMessage requestMessage)?

Apart from that, the proposal looks good to me.

[martintmk]

Hey @iliar-turdushev

As I understand, SetRequestMessage will be useful for the Hedging handler when a user wants to replace the request object with a custom one, right?

Yup, they can also access the request message, extract some information and use it for telemetry. The latter was my primary motivation for these APIs.

Do we want to make the second argument of the SetRequestMessage method non-nullable, i.e. SetRequestMessage(this ResilienceContext context, HttpRequestMessage requestMessage)?

I think we shall still keep the option to "reset" the request message. So my vote would be to keep the nullable option.

[RussKie]

@joperezr @geeknoid any thoughts or objections?

[geeknoid]

LGTM.

[RussKie]

Approved.

@martintmk feel free to send a pull-request with the necessary changes, if you fancy it.

[iliar-turdushev]

The implementation is blocked because of the following issue in Polly App-vNext/Polly#2299. The issue doesn't allow us to make requestMessage argument of SetRequestMessage nullable.

public static void SetRequestMessage(this ResilienceContext context, HttpRequestMessage? requestMessage);

@martintmk Do we really want to allow requestMessage be nullable? In our implementation if ResilienceContext doesn't include requestMessage or if its values is null then we throw an exception. Do we want to allow our customers to create a situation that will lead to an exception?

[martintmk]

The implementation is blocked because of the following issue in Polly App-vNext/Polly#2299. The issue doesn't allow us to make requestMessage argument of SetRequestMessage nullable.

public static void SetRequestMessage(this ResilienceContext context, HttpRequestMessage? requestMessage);

@martintmk Do we really want to allow requestMessage be nullable? In our implementation if ResilienceContext doesn't include requestMessage or if its values is null then we throw an exception. Do we want to allow our customers to create a situation that will lead to an exception?

Here, I wanted to mimic the behavior of SetPolicyExecutionContext:

https://github.com/dotnet/aspnetcore/blob/d691d6a72d3e82114987fe637643fcc209c5d245/src/HttpClientFactory/Polly/src/HttpRequestMessageExtensions.cs#L50

Another scenario where setting the null can be valuable is the reset semantics. Caller can reset the message and the hedging stack can re-initialize it.