Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: Customize Polly policies #264

Closed
mjrousos opened this issue Mar 7, 2018 · 6 comments

Comments

@mjrousos
Copy link

@mjrousos mjrousos commented Mar 7, 2018

Feature Request

This isn't urgent, but it would be great if we could somehow customize Polly policies used for some reroutes. As examples, I'd like to be able to add a retry policy or change the callback used when the circuit breaker opens.

It would be easy to add my own policies in a DelgateHandler but then they would apply to all reroutes (which is undesirable since one bad API would open the circuit breaker for all).

Perhaps there's a way we could inject Policy[] via DI to override the defaults?

@TomPallister

This comment has been minimized.

Copy link
Member

@TomPallister TomPallister commented Mar 7, 2018

@mjrousos Yeh this is a useful feature. I will have a look ASAP.

@TomPallister

This comment has been minimized.

Copy link
Member

@TomPallister TomPallister commented Mar 9, 2018

Maybe we want ReRoute specific delegating handlers, do you agree? I had this functionality requested before.

So we need some way of mapping ReRoutes to handlers, the easiest way to do this would be to have an array of delegating handlers on the ReRoute in configuration.json.

Maybe on the ReRoute we have

"Handlers": [
    "Tracer",
    "PollyOne"
]

then in Ocelot

services.AddOcelot()
    .AddDelegatingHandler<PollyOne>()
    .AddDelegatingHandler<Tracer>()
    .AddDelegatingHandler<Foo>();

When the http client is built for the ReRoute we pick out the delegating handlers that matched the string in json.

Seems simple enough!

TomPallister pushed a commit that referenced this issue Mar 9, 2018
Tom Gardham-Pallister
@mjrousos

This comment has been minimized.

Copy link
Author

@mjrousos mjrousos commented Mar 9, 2018

Yea, that would do the trick. In this model, I could put whatever custom Polly policies I want in a delegating handler and then use it with particular reroutes. A couple concerns, though -

  • Some delegating handlers would still be useful to apply globally, so we may need a way to specify in AddDelegatingHandler if the handler should always apply or if it should only reply when requested.
  • In the Polly case that I'm thinking of specifically, I may want to create one delegating handler with a circuit breaker, for example, and request it on multiple reroutes via "Handlers": [] but I won't want that circuit breaker opening for one of the reroutes to affect the other. So, it will be good if each reroute that uses a delegating handler could get its own instance (so that they can have separate instances of policies or other local state).
@TomPallister

This comment has been minimized.

Copy link
Member

@TomPallister TomPallister commented Mar 9, 2018

Cool sounds like a plan...I’ll make sure each reroute gets its own!

TomPallister pushed a commit that referenced this issue Mar 9, 2018
…re this is correct api for users yet
TomPallister added a commit that referenced this issue Mar 10, 2018
TomPallister added a commit that referenced this issue Mar 10, 2018
TomPallister added a commit that referenced this issue Mar 10, 2018
* #264 added handlers to config

* #264 added global handlers object and defaut param for method, not sure this is correct api for users yet

* #264 Can now add all sorts of delegating handlers in all sorts of ways

* +semver: breaking #264
@TomPallister

This comment has been minimized.

Copy link
Member

@TomPallister TomPallister commented Mar 10, 2018

@mjrousos OK ive just pushed version 5.0.0 that has changes that should let you do all of the things we talked about (hopefully). Let me know if this isn't the case.

Updated docs here hopefully they make sense.

TomPallister added a commit that referenced this issue Mar 10, 2018
@mjrousos

This comment has been minimized.

Copy link
Author

@mjrousos mjrousos commented Mar 10, 2018

Awesome; thanks! I'll check it out and let you know. Thanks for the quick turn-around!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.