Feign: Make Hystrix Command names more unique

[ryanjbaxter]

By default Feign will create Hystrix Commands that use the method name as the key. It is possible to have two Feign clients with the same method names that are completely separate Hystrix Commands. However since the the method names are the same the underlying Hystrix Command cache will see these both as the same command. We can do a little better at our default key naming convention by including the class name in the key we use as well.

This should be a simple change in the code here
https://github.com/OpenFeign/feign/blob/master/hystrix/src/main/java/feign/hystrix/HystrixInvocationHandler.java#L90

For more information see the discussion in this issue in the Hystrix project.

[adriancole]

Feign.configKey was made exactly for this reason

[ryanjbaxter]

@adriancole of course there is a solution to this problem ;) how is Feign.configKey used? I did a quick Google search but I dont see any doc.

[adriancole]

Feign works by parsing an interface into MethodMetadata. This type is used to help assist implementations on how to control things.

Right now, configKey is used to create the method dispatch map, and it is also used a tag in logging. Ex below:

    if (logLevel != Logger.Level.NONE) {
      logger.logRequest(metadata.configKey(), logLevel, request);
    }

My suspicion is that you could change hystrix to take advantage of this MethodMetadata.configKey, which is set at contract time by Feign.configKey.

---

Internals of feign won't end up in google. For things like this it is best to do is look at the javadocs and/or do a reference check.

ex. here's a convenience paste of the javadoc:

  /**
   * <br> Configuration keys are formatted as unresolved <a href= "http://docs.oracle.com/javase/6/docs/jdk/api/javadoc/doclet/com/sun/javadoc/SeeTag.html"
   * >see tags</a>. <br> For example. <ul> <li>{@code Route53}: would match a class such as {@code
   * denominator.route53.Route53} <li>{@code Route53#list()}: would match a method such as {@code
   * denominator.route53.Route53#list()} <li>{@code Route53#listAt(Marker)}: would match a method
   * such as {@code denominator.route53.Route53#listAt(denominator.route53.Marker)} <li>{@code
   * Route53#listByNameAndType(String, String)}: would match a method such as {@code
   * denominator.route53.Route53#listAt(String, String)} </ul> <br> Note that there is no whitespace
   * expected in a key!
   *
   * @param targetType {@link feign.Target#type() type} of the Feign interface.
   * @param method invoked method, present on {@code type} or its super.
   */

I'll update that to double-link back to MethodMetadata.configKey, but duplicating javadoc elsewhere isn't that keen as not only would it make for drift, but the only people who'd be looking at this would be doing something custom (and therefore have access to configKey's javadoc).

  /**
   * @see Feign#configKey(Class, java.lang.reflect.Method)
   */
  public String configKey() {
    return configKey;
  }

[adriancole]

added more explanation around configKey here: https://github.com/OpenFeign/feign/pull/437

[ryanjbaxter]

@adriancole maybe I am just slow, but I still don't see how to do the following

1. How the developer would customize the configKey value when they create a Feign Client
2. How that value would then effect the key used to store the hystrix command in the cache. The code in HystrixInvocationHandler seems pretty strait forward in that it uses the method name as the key.

[adriancole]

Let's first unpack your original request.

You've mentioned that there's a conflict on the method name.. I assumed it was because of lack of context (Ex two different interfaces with the same method name). If that were the case, using a command key with something more specific than the method name would help. (ex configKey which also includes type name and args)

The developer (the one writing the change to feign) would do something as simple as changing the command key from method.getName() to something else (ex using the configKey function I mentioned).

Let's get more specific about the type of clash we are trying to avoid. Would having simple name of the type and also the args eliminate the clash? If so, a very simple change should work. If not, we need to figure out a way to eliminate the clash.

[ryanjbaxter]

Actually in my specific situation it was an '@HystrixCommand' and a feign client method clashing but yes 2 feign client s with the same method names would run into the same problem.

When I said "developer" in my comment above I was referring to the developer using feign in their app not the one making the fix ;)

I think we just need to use the fully qualified class name + method name but including the arts would not be a bad idea as well. If you agree I can work on the fix.

[apischan]

Guys, I have found the pull request that can solve this problem #360 but I see that the thread is stuck. Whether any path forward with it expected? It is actually will be nice to have annotation from that pull request. It will automatically solve this issue as well.

[ryanjbaxter]

I agree it would provide a solution to the problem, but I would also like to provide a solution that would work a little better out of the box (without needing the add additional config to the annotation.

[apischan]

@ryanjbaxter I totally agree because I accidently came into failure case using out of the box configuration. It is easy to make a mistake and this is not obvious at first sight where the issue is.

[adriancole]

Food for thought. Including args assumes args make the command different..
ex list vs list(startmarker). If that helps more than hurts, lets do it.

If main thing is to not clash with other things, we could name it
"feign#Class.method" or something. Then it would be obvious this is feign.
That said not sure the prefix helps in dashboards etc ;)

Your call and agree the developer would need to know the convention (ie
README and javadoc)

Appreciate the offer for help, Ryan!

[spencergibb]

What if we provide a sane default and have an option in the builder to customize?

[adriancole]

I'd prefer solving the problem independently of making things
extensible. each extension point (or more generally each new type) is
a liability to the codebase.

That said, I'm ok with this as long as we try to remain light on
type-counts. ex be very specific with what sort of customization we're
doing and not have to make several similarly named interfaces (as code
like this usually ends up lightly tested).

Ex the following could be meaningfully created at contract time by
correlating things from methodMetadata.configKey

HystrixCommand.Setter create(Target target, String configKey) {
  String groupKey = target.name();
  String commandKey = configKey; // or some slice of it
  return HystrixCommand.Setter
    .withGroupKey(HystrixCommandGroupKey.Factory.asKey(groupKey))
    .andCommandKey(HystrixCommandKey.Factory.asKey(commandKey));
}

[ryanjbaxter]

Here is a pretty basic implementation that incorporates some of the things we have talked about

[adriancole]

going to give a crack at extracting this stuff such that it can be processed up-front