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 Dashboard Eureka integration improvements #1336

Merged

Conversation

kennedyoliveira
Copy link
Contributor

Hello!

Did some improvements on Hystrix Dashboard Eureka Integration, hope it helps!

The following was improved:

  • Added remove button for added streams
  • Refactored the javascript code on index
  • Updating url on changing stream type
  • Updated the url scheme to consider the port on eureka, instead of using only 8080, useful when using ephemeral ports, for example on local dev deployment.
  • Update title for application name, after you select the application, the title will be updated with it's name
  • Updated the list of apps list all the applications instances
  • Updated the list to group the instances by application name
  • Added a group called "applications" that will have the first instance of each application, maintaining a similiar behaviour to the the legacy implementation, although the legacy behaviour used the last instance ip and this new one use the first one, i guess this will be more to use applications like turbine, that will have only one instance usually.

I guess these changes improves the overall UX, hope it helps!

New app listing:
fullscreen_8_28_16__1_15_am

Remove stream:
hystrix_dashboard

- Refactoring code
- Grouping instances by services
- Added a new group for the first instance of each server
- Updating url on chaning stream type
- Update title for with application name
@mattrjacobs
Copy link
Contributor

These changes look good to me. Anybody else that uses hystrix-dashboard willing to try this out and give @kennedyoliveira feedback?

/cc mukteshkrmishra

@mukteshkrmishra
Copy link

@mattrjacobs @kennedyoliveira : Changes looks good to me ..just tested it.

@spencergibb
Copy link
Contributor

I'd like to take a look

@mattrjacobs
Copy link
Contributor

Thanks @mukteshkrmishra / @spencergibb for volunteering!

@kennedyoliveira
Copy link
Contributor Author

@spencergibb any feedback?

@kennedyoliveira
Copy link
Contributor Author

@mattrjacobs can you merge it?

@mattrjacobs
Copy link
Contributor

Sure thing. Missed @spencergibb's thumbs-up - those don't trigger my email alerts :(

Thanks @kennedyoliveira for the contribution, and thanks @spencergibb and @mukteshkrmishra for the review!

@mattrjacobs mattrjacobs merged commit d838f4d into Netflix:master Sep 8, 2016
@kennedyoliveira
Copy link
Contributor Author

@mattrjacobs Your welcome! I missed the thumbs-up too haha.

@mukteshkrmishra
Copy link

@kennedyoliveira 👍 Here you go :)

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