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

[hystrix-contrib/hystrix-clj] Making Command keys quantified by namespaces. #181

Merged

Conversation

josephwilk
Copy link
Contributor

This improves the Hystrix dashboard reporting information.

If you have two fns with the same name in different namespaces,
their command key will be the same. This messes up any monitoring using the Hystrix dashboard (the two conflicting commands were trying to write to the same graph I suspect).

It is also confusing that circuit breakers are only shown by their command key.
Namespaces provide more useful information in reporting.

My only goal is to improve reporting. If this is better actioned upon in the Hystrix dashboard project, I'm happy to add a patch there.

Thanks,
Joe

This improves the Hystrix dashboard reporting information. 

If you have two fns with the same name in different namespaces,
their command key will be the same. This messes up any monitoring using the Hystrix dashboard.
It is also confusing that circuit breakers are only shown by their command key. 
Namespaces provide more useful information in reporting.
@cloudbees-pull-request-builder

Hystrix-pull-requests #47 ABORTED

@daveray
Copy link
Contributor

daveray commented Sep 25, 2013

I've been thinking about making this change as well. Note that you can override the name in the command metadata as well. A few considerations:

  • This is a breaking change. If existing users have configured commands based on the current naming scheme, that configuration will be silently ignored
  • Do command names with dots and slashes cause any issues with configuration or the dashboard? We use Archaius for config and a property named hystrix.command.com.netflix.my-app/search.execution.isolation.thread.timeoutInMilliseconds might seem strange.

Maybe @benjchristensen can comment on the second issue?

@benjchristensen
Copy link
Contributor

I don't know if Archaius would have issues with that.

I believe this line in the dashboard javascript will escape the characters that would cause issues: https://github.com/Netflix/Hystrix/blob/master/hystrix-dashboard/src/main/webapp/components/hystrixCommand/hystrixCommand.js#L91

Another consideration is that if the name is longer it will get cropped on the UI.

@daveray
Copy link
Contributor

daveray commented Sep 27, 2013

I'll do a little experiment to see how archaius likes slashes and dots in command names. If that's cool, I'm good with merging. For the next release, we'll need to communicate clearly that this change is there, it will affect existing configurations.

Personally, in both Java and Clojure, I've taken to always explicitly naming the commands. Otherwise, it's too easy to break things that are auto-named when refactoring.

@daveray
Copy link
Contributor

daveray commented Sep 27, 2013

Everything worked fine. Merge away. Thanks @josephwilk

benjchristensen added a commit that referenced this pull request Sep 27, 2013
[hystrix-contrib/hystrix-clj] Making Command keys quantified by namespaces.
@benjchristensen benjchristensen merged commit 1b144fc into Netflix:master Sep 27, 2013
@benjchristensen
Copy link
Contributor

Can one of you draft some release notes to include with this change?

@daveray
Copy link
Contributor

daveray commented Sep 27, 2013

Release note draft:

Breaking Change for hystrix-clj: The default command key generated by the defcommand macro is now the fully qualified name. For example:

(ns com.netflix.my-service)
(defcommand my-command ...)

Will create a HystrixCommand with key com.netflix.my-service/my-command instead of just my-command. This has the following implications:

  • Existing Archaius or other name-based configurations must be updated.
  • Depending on the size of the namespace, the dashboard may now truncate the name of the command when displayed.

Note that the command key can be set manually with the :hystrix/command-key option:

(defcommand my-command
  {:hystrix/command-key "my-command"}
  ... command body ...)

Now the command key will be just "my-command", i.e. the behavior of hystrix-clj before this change.

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

Successfully merging this pull request may close these issues.

4 participants