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

[Turbine] Turbine controller #2223

Merged
merged 1 commit into from Apr 25, 2018

Conversation

@Aloren
Copy link
Contributor

Aloren commented Aug 23, 2017

I think it makes sense to have some sort of endpoint that gives users information about currently monitored clusters. Currently if you have many services with different configured clusters it's very difficult to know which streams are actually working and what are the links to them.
For now I added only cluster name and link to the monitoring stream just to give an idea. WDYT?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 23, 2017

Codecov Report

Merging #2223 into master will increase coverage by 8.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2223      +/-   ##
==========================================
+ Coverage   68.05%   76.11%   +8.05%     
==========================================
  Files         230      165      -65     
  Lines        8118     5418    -2700     
  Branches     1013      745     -268     
==========================================
- Hits         5525     4124    -1401     
+ Misses       2208     1017    -1191     
+ Partials      385      277     -108
Impacted Files Coverage Δ
.../eureka/server/event/EurekaServerStartedEvent.java
...lix/metrics/spectator/SpectatorMetricServices.java
...cloud/netflix/turbine/SpringAggregatorFactory.java
.../cloud/netflix/eureka/server/InstanceRegistry.java
...rk/cloud/netflix/eureka/EurekaDiscoveryClient.java
...k/cloud/netflix/eureka/EurekaClientConfigBean.java
.../cloud/netflix/eureka/server/CloudJacksonJson.java
...flix/eureka/server/InstanceRegistryProperties.java
...veryClientConfigServiceBootstrapConfiguration.java
.../cloud/netflix/eureka/server/EurekaController.java
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 885425e...22f37d4. Read the comment docs.

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented Aug 23, 2017

I am questioning why you wouldnt know which clusters are configured if you configured them to begin with.

@Aloren

This comment has been minimized.

Copy link
Contributor Author

Aloren commented Aug 23, 2017

Let me explain all the features that we are currently in need in Turbine. One of them is having dynamic monitoring dashboard.

If we have ~10 microservices -- services monitoring is not an issue. But as soon as number of microservices grows -- amount of configuration rapidly increases:

  1. You need to add cluster configuration for your service to: turbine.aggregator.clusterConfig
  2. You need to add service configuration to: turbine.appConfig

Currently we have Turbine dashboard static page, that stores all monitoring links, but this is not convenient, because each time we add new service with hystrix stream we need to manually add valid monitoring link to it. It means when adding new microservice to Turbine we also have one more additional step: add configuration for Turbine dashboard.

By this pull request I would like to omit the last step in our configuration: retrieving configured streams from Turbine via REST API gives us possibility to remove static configuration from Turbine Dashboard.

Copy link
Contributor

ryanjbaxter left a comment

This looks good to me!

@Aloren

This comment has been minimized.

Copy link
Contributor Author

Aloren commented Sep 1, 2017

Ok, thanks!. I will add tests for the feature in a couple of days then.

@spencergibb

This comment has been minimized.

Copy link
Member

spencergibb commented Sep 1, 2017

Would it make more sense for this to be an actuator endpoint rather than a bare controller?

@Aloren

This comment has been minimized.

Copy link
Contributor Author

Aloren commented Sep 1, 2017

Hm, I'm not sure. If in future UI will be added for this information (smth like Eureka dashboard), then it is better not to stick to management port, I think. If you insist I will change to actuator endpoint, currently it does not matter.

@spencergibb

This comment has been minimized.

Copy link
Member

spencergibb commented Sep 1, 2017

I won't support adding a turbine UI here, but that's beside the point (netflix no longer supports turbine). /turbine.stream isn't an actuator, so I'm fine with it not being actuator.

@Aloren Aloren force-pushed the Aloren:feature/turbine-controller branch to 22f37d4 Sep 7, 2017
@spencergibb spencergibb added this to the 2.0.0.RELEASE milestone Apr 25, 2018
@spencergibb spencergibb merged commit 60407e4 into spring-cloud:master Apr 25, 2018
1 of 2 checks passed
1 of 2 checks passed
ci/circleci Your tests failed on CircleCI
Details
ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details
@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented May 7, 2018

@Aloren wondering if you could provide some clarification for me, as I cannot seem to get this to work. The RestController is not being created for me when I have Turbine on the classpath, and there is no TurbineInformationService bean created in any configuration. So does the user need to do this all themselves in order to get this to work? At very least we need some docs around this.

@Aloren

This comment has been minimized.

Copy link
Contributor Author

Aloren commented May 7, 2018

@ryanjbaxter i haven't seen this pr was merged. it doesn't have any tests or configurations for setting up any beans, so i can not guarantee that it works. do you want me to create another PR with cleanup for this one?

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented May 11, 2018

@Aloren Yes please, that would be great!

I created an issue for this #2929

@Aloren

This comment has been minimized.

Copy link
Contributor Author

Aloren commented May 14, 2018

@ryanjbaxter is there any deadline for the changes? :) so i could plan my time )

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented May 14, 2018

It would be ideal to have it working in Finchley.RC2 that is scheduled to be released at the end of the week. However if you cant make that work then surely before Finchley.RELEASE.

@Aloren

This comment has been minimized.

Copy link
Contributor Author

Aloren commented May 23, 2018

Sorry, didn't have time for the changes. Will do this week.

@ryanjbaxter

This comment has been minimized.

Copy link
Contributor

ryanjbaxter commented May 23, 2018

@Aloren I just submitted a PR for this, see #2966

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