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

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

Closed

Conversation

micheal-swiggs
Copy link
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.

@@ -61,12 +61,14 @@

private final EurekaClient eurekaClient;
private final Expression clusterNameExpression;
private final boolean combineHostPort;
Copy link
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
Member

Have you signed the contributor's agreement?

@micheal-swiggs
Copy link
Contributor Author

Just signed it. My confirmation number is 141620151006073646.

@spencergibb
Copy link
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
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
Member

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

@micheal-swiggs
Copy link
Contributor Author

I've added the tests and fixed them.

@spencergibb
Copy link
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
Contributor Author

I will try.

@micheal-swiggs
Copy link
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

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