My goal is to avoid polluting System.Net.Http. We could also use Microsoft.Extensions.Http but it seems more natural to use the Polly namespace since you'd likely have it already added if you're interested in the context. Thoughts?

With the metapackage approach, it means that a lot of users have will have this package available without realizing it. This is ultimately going to be included in the Microsoft.AspNetCore.All package.

Re:

goal is to avoid polluting System.Net.Http. [...] seems more natural to use the Polly namespace

👍 So the extension methods would not appear unless someone had using Polly;, not confusing those not interested.

Thoughts on naming? I also considered GetPolicyContext, which I kinda like better on second thought.

We could also give this a scarier name like GetPolicyExecutionContext to act as salt.

My vote on naming is for GetPolicyExecutionContext(): reinforces the idea that the context is execution-scoped. If so, should the change flow elsewhere also?, eg:

internal static readonly string PolicyExecutionContextKey = "PolicyExecutionContext";

Sure, will make this change.

Suggestions how to create the executioncontextid?

@rynowak Just for curiosity, why is there always a Context creation for each request execution (with Context and string allocation), even for uses that don't use it? Why didn't you prefer something like

public static Context GetPollyContext(this HttpRequestMessage request)
{
   if (!request.Properties.TryGetValue(PollyContextKey, out var context))
   {
       context = new Context(Guid.NewGuid().ToString());
       request.Properties[PollyContextKey] = context;
   }
   return context;
}

so the context is allocated only if the user ask for it.

GetPollyContext() is doing the TryGet() here and SetPollyContext() is going the set when one needs to be made. In that example it's not really getting, because it sets if it's not already there. Your example would be more EnsurePollyContext().

Suggestions how to create the executioncontextid?

If the user has attached no context to the request, just:

context = new Context();

Context.ExecutionKey is the key set via constructor. This represents the call site usage of the policy. Not-set is allowed; if the user has not chosen to tag Polly logging/telemetry by call site tags (by using request.SetPollyContext(new Context("MyUsageTag"));), we should not set it to something arbitrary.

Compare: Context.ExecutionGuid is the separate, per-execution-unique correlation id. That is automatically initialised by Polly.

EDIT for clarity: A context instance is needed, so that the same Context instance with same Context.ExecutionGuid flows through the whole execution. But we shouldn't set the Context.ExecutionKey if the user doesn't. I think the naming of those two keys on Context doesn't help here, and we could improve it.

@Yves57 Re:

why is there always a Context creation for each request execution [...] even for uses that don't use it?
Why didn't you prefer [...] so the context is allocated only if the user ask for it?

Polly uses a Context any time executing through a policy, to provide context to telemetry and logging, so the allocation isn't extra; Polly would allocate it later internally if it wasn't done here.

By scoping the context to the HttpRequestMessage, we ensure that the same Context instance is used through the whole chain of DelegatingHandlers when the user has configured multiple PolicyHttpMessageHandler instances. This might be common, for example using both a circuit-breaker and retry policy: .AddPolicyHandler(/* some circuit-breaker */).AddPolicyHandler(/* some retry policy */).

While the motivation was to make that Context consistent through a chain of PolicyHttpMessageHandlers, it will actually mean fewer allocations: if Context wasn't scoped to HttpRequestMessage, the execution through each policy would generate a fresh Context, meaning more allocations.

Good question; hope this explanation helps.

@reisenberger Thank you for the precise answer! Looking in the Polly code, I have seen that the "default" context that would be allocated. So I understand now that the best is to allocate it here.

@reisenberger - FYI it will be new Context(null) the no-arg version is internal.

Note that I'm explicitly not cleaning up the context here. It's not clear that there's any value to that, but I'm interested in thoughts.

I have a suspicion that we might want to... but it would only affect some truly weird scenarios. Like if someone has implemented their own retry support, but they aren't setting the context themself.

explicitly not cleaning up the context here

It seems correct (in fact important) that we don't clean up the context. This line in execution could be hit within the looping flowing from an enclosing retry policy. We'd want the same context to hold through all retries, so we shouldn't (for example) set it to null here. (That iiuc your thought re 'clean up context' - come back on this if I'm missing something, or if you've seen scenarios I haven't that might not play well with that.)

Ok good, sounds like this is right then.

Adding this seems like a good refactor. Anyone who needs to subclass can override to run functionality 'inside' the policy. Also allowed me to clean up tests a bit.

Test as is does not check that the correct context is passed to SendAsync? SUT could push any other context onto HttpRequestMessage before calling SendAsync(...), pop it off again after, and the test would still pass? Could strengthen by tracking Context contextPassedToSendAsync;

Sure, this got missed somehow.

contextPassedToSendAsync = c;

This comment is a bit stale now, but I addressed the underlying problem

Assert.AreSame(context, contextPassedToSendAsync);

This comment is a bit stale now, but I addressed the underlying problem

Awesome to see the Context support going in; great that we can add xtn methods to HttpRequestMessage to hide the details.