Skip to content

Enables monitoring of multiple services on a single physical box.#574

Closed
micheal-swiggs wants to merge 7 commits into
spring-cloud:masterfrom
micheal-swiggs:v1.1.0.M1-fixes
Closed

Enables monitoring of multiple services on a single physical box.#574
micheal-swiggs wants to merge 7 commits into
spring-cloud:masterfrom
micheal-swiggs:v1.1.0.M1-fixes

Conversation

@micheal-swiggs

Copy link
Copy Markdown
Contributor

By setting turbine.instanceInsertPort.default=false and turbine.combineHostPort=true services registered with Eureka running on the same server(ie same hostname) can be monitored with turbine. I'm not sure if this is a good way of enabling this.

Another option is to change TurbineConfiguration to:

@Autowired
private InstanceDiscovery iDiscovery;

@Override
public void start() {
    PluginsFactory.setClusterMonitorFactory(new SpringAggregatorFactory());
    PluginsFactory.setInstanceDiscovery(iDiscovery);
    TurbineInit.init();
}

And allow the user to inject their own InstanceDiscovery implementation, e.g a subclass of EurekaInstanceDiscovery.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use tabs.

@spencergibb

Copy link
Copy Markdown
Member

Have you signed the contributor's agreement?

@micheal-swiggs

Copy link
Copy Markdown
Contributor Author

Just signed it. My confirmation number is 141620151006073646.

@spencergibb

Copy link
Copy Markdown
Member

@micheal-swiggs I changed turbine recently. I thought I'd just add your changes. When I added this test to EurekaInstanceDiscoveryTest:

    @Test
    public void testCombineHostPort() {
        turbineProperties.setCombineHostPort(true);
        EurekaInstanceDiscovery discovery = getEurekaInstanceDiscovery();
        String appName = "testAppName";
        int port = 8080;
        String hostName = "myhost";
        InstanceInfo instanceInfo = builder.setAppName(appName)
                .setHostName(hostName)
                .setPort(port)
                .build();
        Instance instance = discovery.marshall(instanceInfo);
        assertEquals("hostname is wrong", hostName+":"+port, instance.getHostname());

        String urlPath = SpringClusterMonitor.ClusterConfigBasedUrlClosure.getUrlPath(instance);
        assertEquals("url is wrong", "http://"+hostName+":"+port+"/hystrix.stream", urlPath);
    }

It fails with

java.lang.RuntimeException: Configured to use port, but port or securePort is not in host attributes

    at org.springframework.cloud.netflix.turbine.SpringClusterMonitor$1.getUrlPath(SpringClusterMonitor.java:108)
    at org.springframework.cloud.netflix.turbine.EurekaInstanceDiscoveryTest.testCombineHostPort(EurekaInstanceDiscoveryTest.java:99)

If you want to rework it so a test like that doesn't fail, go for it. Otherwise, you can now create your own InstanceDiscovery beans.

@micheal-swiggs

Copy link
Copy Markdown
Contributor Author

I will try to rework this. So if I add this test into my branch I should be able to reproduce the error?

@spencergibb

Copy link
Copy Markdown
Member

You should. the getUrlPath fails if there is no port metadata.

@micheal-swiggs

Copy link
Copy Markdown
Contributor Author

I've added the tests and fixed them.

@spencergibb

Copy link
Copy Markdown
Member

@micheal-swiggs, just looking at this and realized you've made this from the M1 branch, can you do rebase to master instead?

@micheal-swiggs

Copy link
Copy Markdown
Contributor Author

I will try.

@micheal-swiggs

Copy link
Copy Markdown
Contributor Author

I was not able to rebase this branch. So I have created a new pull request forked from master with the changes of this pull request.

@codependent

Copy link
Copy Markdown

Last November I came across this same problem and found an open issue in Turbine's project.

I finally submitted a PR addressing it. The changes introduced here definitely fixed the problem but in my opinion it would make sense that the solution were on Turbine's side. Why? Because this PR only works when you have Eureka, but doesn't when using other discovery server such as Consul.

What do you think?

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.

3 participants