Skip to content
This repository has been archived by the owner on Nov 17, 2018. It is now read-only.

Execution-scoped data in the Polly-HttpClientFactory integration: enabling rich and correct telemetry, and CachePolicy #62

Closed
reisenberger opened this issue Feb 20, 2018 · 3 comments
Assignees
Milestone

Comments

@reisenberger
Copy link
Contributor

Polly Policy instances can be used across multiple call sites for intended functional outcomes. This is a good fit for named HttpClient configurations on HttpClientFactory with a BaseUri eg api.contoso.com indicating all calls will be to endpoints on api.contoso.com.

To complement re-use, Polly provides an execution-scoped Context which travels with every execution. This:

(a) provides a correlation id unique to each execution, allowing logging and telemetry to correlate the events particular to a single execution;

(b) distinguishes different call sites at which Policy instances may be being used, allowing logging and telemetry to correlate statistics particular to a single usage site/call path;

  • For instance, it is typical to use the same circuit-breaker instance for all endpoints on api.contoso.com, but when analysing failures, it can be very useful to extract that call path A tended to be healthy but call path B tended to require retries, break circuits, etc.

(c) allows the functioning of CachePolicy. With CachePolicy, one typically configures a single CachePolicy instance, then Context supplies the key that the policy should use for caching items on that particular call path.

The execution-scoped Polly.Context is already made available to policy delegate hooks (eg onRetry, onBreak etc) and will also be available to forthcoming raw telemetry events.


Without HttpClientFactory doing anything around this, correlation ids (a) would be inconsistent in cases where multiple Polly policies are applied by layering DelegatingHandlers (each PolicyHttpMessageHandler layer would generate a fresh guid). And features (b) and (c) would not be available.

Options for supporting Polly.Context in PolicyHttpMessageHandler

Here within PolicyHttpMessageHandler we could:

Polly.Context pollyExecutionContext = request.Properties[PollyContext] ?? new Polly.Context(); // [1]
request.Properties[PollyContext] = pollyExecutionContext; // [2]  

return _policy.ExecuteAsync((context, token) => base.SendAsync(request, token), pollyExecutionContext, cancellationToken); // [3]

where:

private static readonly string PollyContext = "PollyExecutionContext"; // [4]

[1] and [2] ensure when there is a chain of delegating handlers, they all use the same context, fixing (a) above. [3] just despatches so that all Polly logging and telemetry has access to the Context.

If we changed [4] to:

public static readonly string PollyContext = "PollyExecutionContext"; // [4b]

then users who desired, would have this usage:

request.Properties[PolicyHttpMessageHandler.PollyContext] = new Polly.Context("MyUsageKey"); // [5] 
var response = client.DoWhateverAsync(request...); // (as usual)

This enables and fixes both use cases (b) and (c) above.


Stepping back: the issue here is that Polly users normally code the fooPolicy.ExecuteAsync(...) call directly, so can pass in a Context. In the HttpClientFactory integration, that call is (necessarily) within PolicyHttpMessageHandler.cs, so the user cannot directly pass in Context.

HttpClient being (presumably) not for modification, HttpRequestMessage.Properties seems like the intended(?)/available place for attaching execution-scoped data.

The extra line, [5], would not be necessary for ordinary usage. It would be an available option, for those who wanted the richer telemetry or to use CachePolicy.


Thoughts? Should this be part of the Polly-HttpClientFactory story? Are there any good alternatives?

@rynowak
Copy link
Member

rynowak commented Feb 21, 2018

I think this seems reasonable. It's something I was wondering about myself.

We might even go further than 4b and just define extension methods that do this in a nicer way.

@rynowak rynowak added this to the 2.1.0 milestone Feb 21, 2018
@rynowak rynowak self-assigned this Feb 21, 2018
@reisenberger
Copy link
Contributor Author

reisenberger commented Feb 21, 2018

We might even go further than 4b and just define extension methods that do this in a nicer way.

Perfect! My concern around [5] was that while sthg like this seemed necessary, it wasn't as pretty as it could be, so if you have licence to add extension methods to wrap-and-hide [4b] / [5], even better.

@rynowak
Copy link
Member

rynowak commented Feb 28, 2018

This is done. We did all of this stuff 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants