integration transport module with spring-mvc

[shenbaoyong]

Describe what this PR does / why we need it

Fixes #1565

Does this pull request fix one issue?

Fixes #1565

Describe how you did it

I implements tow interfaces of spring-mvc(SentinelApiHandlerAdapter-HandlerAdapter SentinelApiHandlerMapping-HandlerMapping), mapping the api of dashboard want to visit to SentinelApiHandler(CommandHandler)。
the part of heartbeat, I just copy from sentinel-transport-netty-http(use HttpClient lib)

Describe how to verify it

1. dowload my code
2. package sentinel-transport-spring-mvc(1.8.1-SNAPSHOT) module to a jar
3. create a web application use spring-mvc or spring-boot
4. import sentinel-core(1.8.0) and sentinel-transport-spring-mvc(1.8.1-SNAPSHOT)
5. registe SentinelApiHandlerAdapter and SentinelApiHandlerMapping as bean to ioc container
6. config the dasboard address 'csp.sentinel.dashboard.server' (use config file or startup parameters)
7. config the user application port 'csp.sentinel.api.port' (use config file or startup parameters)
8. write some sentinel example
9. startup the user application
10. visit user web appliation api to trigger sentinel initialization
11. then user web application will send hearbeat to dasboard, dasboard can get info from user application by send http request

Special notes for reviews

[cdfive]

Good idea! I think it can really solve the problem in #1565.
I tried with sentinel-demo-spring-webmvc in the project, and it works, but some configurations needs to be added, such as

@Bean
public SentinelApiHandlerMapping sentinelApiHandlerMapping() {
    return new SentinelApiHandlerMapping();
}

@Bean
public SentinelApiHandlerAdapter sentinelApiHandlerAdapter() {
    return new SentinelApiHandlerAdapter();
}

Besides, -Dcsp.sentinel.api.port=10000 need to be added, since SpringMvcHttpCommandCenter doesn't try to find a port like SimpleHttpCommandCenter does, and the client port is null by default, it won't send heartbeat message.
They are also mentioned in step 5, 7.

Could you please add a quick-start demo for this new module sentinel-transport-spring-mvc? which may help more users.
Another way is that just modify demo sentinel-demo-spring-webmvc, use sentinel-transport-spring-mvc instead of sentinel-transport-simple-http, which do you think is better? @shenbaoyong @sczyh30 @jasonjoo2010

[cdfive]

How about making spring-webmvc,javax.servlet-api scope provided, since user may use different version of spring or spring-boot, the version may conflicts. Another way is that user can exclude them manually.

[shenbaoyong]

yes I will change their scope to provided

[shenbaoyong]

I tend to create new demo module. I think sentinel-transport-spring-mvc and sentinel-spring-webmvc-adapter have different focus, it is better to separate.

[cdfive]

It had a point, a separate demo will make it easier for new users to understand how to use this new module.

[cdfive]

A small suggestion for reference and discussion:

If user choose to use sentinel-transport-spring-mvc, his project must use spring-webmvc, and there's a good chance he's using spring-boot, since spring-boot has been widely used in java web development.

In step7 of how to verify it, we need to config the user application port 'csp.sentinel.api.port',
in web project, that is the HTTP port, usually server.port indicated.
If this step can be done automatically, this module will be more convenient to use.

We can get the web port and config it with TransportConfig of sentinel like this:

@Component
public class SpringMvcHttpHeartbeatSender implements HeartbeatSender, ApplicationListener<WebServerInitializedEvent> {
...
@Override
public void onApplicationEvent(WebServerInitializedEvent event) {
    int port = event.getWebServer().getPort();
    RecordLog.info("[SpringMvcHttpHeartbeatSender]get the web port: {}", port);
    TransportConfig.setRuntimePort(port);
}

The ApplicationListener is in spring-context dependency, but the WebServerInitializedEvent is in spring-boot dependency. User may not use spring-boot, only use spring,spring-webmvc and deployed with separate tomcat server(not embeded tomcat) especially for some old projects. This point may need to pay attention and discuss.

[shenbaoyong]

I find a class named ServerPortInfoApplicationContextInitializer, it is provided by spring-boot and is effective by default. he listen WebServerInitializedEvent and set the port to environment use key 'local.server.port'

public class ServerPortInfoApplicationContextInitializer implements
		ApplicationContextInitializer<ConfigurableApplicationContext>, ApplicationListener<WebServerInitializedEvent> {
	...

	@Override
	public void onApplicationEvent(WebServerInitializedEvent event) {
		String propertyName = "local." + getName(event.getApplicationContext()) + ".port";
		setPortProperty(event.getApplicationContext(), propertyName, event.getWebServer().getPort());
	}

	private String getName(WebServerApplicationContext context) {
		String name = context.getServerNamespace();
		return StringUtils.hasText(name) ? name : "server";
	}
	...
}

so I think we can get port from environment use key 'local.server.port'.
I suggest let SentinelApiHandlerMapping implements EnvironmentAware interface,

public class SentinelApiHandlerMapping extends AbstractHandlerMapping implements EnvironmentAware {
    ...
    private static Environment env;
    
    @Override
    public void setEnvironment(Environment environment) {
        env = environment;
    }

    /**
     * @see org.springframework.boot.web.context.ServerPortInfoApplicationContextInitializer
     */
    public static String getPort() {
        return env.getProperty("local.server.port");
    }
    ...
}

I suggest when get port, the user config hava High priority, in some cases application actual running ports is different from external system accessed

.setParameter("port", TransportConfig.getPort() != null ? TransportConfig.getPort() : SentinelApiHandlerMapping.getPort())

[cdfive]

Yes, user config should have Higher priority.

Another way is:

if (TransportConfig.getPort() == null) {
    TransportConfig.setRuntimePort(env.getProperty("local.server.port"));
}

.setParameter("port", TransportConfig.getPort())

This may more clear and readable, since we can call TransportConfig.getPort() to get the sentinel client port anywhere else simply.

.setParameter("port", TransportConfig.getPort() != null ? TransportConfig.getPort() : SentinelApiHandlerMapping.getPort())
is okay as well, works as exceped, the main idea is to get the web port of the project, please feel free to choose.

[shenbaoyong]

Yes, user config should have Higher priority.

Another way is:

if (TransportConfig.getPort() == null) {
    TransportConfig.setRuntimePort(env.getProperty("local.server.port"));
}

.setParameter("port", TransportConfig.getPort())

This may more clear and readable, since we can call TransportConfig.getPort() to get the sentinel client port anywhere else simply.

.setParameter("port", TransportConfig.getPort() != null ? TransportConfig.getPort() : SentinelApiHandlerMapping.getPort())
is okay as well, works as exceped, the main idea is to get the web port of the project, please feel free to choose.

   /**
     * Get Server port of this HTTP server.
     *
     * @return the port, maybe null if not configured.
     */
    public static String getPort() {
        if (runtimePort > 0) {
            return String.valueOf(runtimePort);
        }
        return SentinelConfig.getConfig(SERVER_PORT);
    }

TransportConfig.getPort method logic, it get runtimePort first, user config second.
so if use TransportConfig.setRuntimePort(env.getProperty("local.server.port")), the user config will not be returned (if the local.server.port is not null).
At the company I work for, For example, I have a application which port is 19001, but all other applicatin want to access it, must use port 80 (if access 19001 will fail)

[cdfive]

if (TransportConfig.getPort() == null) {
    TransportConfig.setRuntimePort(env.getProperty("local.server.port"));
}

so if use TransportConfig.setRuntimePort(env.getProperty("local.server.port")), the user config will not be returned (if the local.server.port is not null).

Before TransportConfig.setRuntimePort there is a judgement, if the web port of application is 19001, and then user's custom config port is 80, the TransportConfig.getPort() will not be null, so the runtimePort will not be set with 19001, is it right?

[shenbaoyong]

Before TransportConfig.setRuntimePort there is a judgement, if the web port of application is 19001, and then user's custom config port is 80, the TransportConfig.getPort() will not be null, so the runtimePort will not be set with 19001, is it right?

ok I get it, and I tend to use this way.
It combine with my approach, the code is this

public class SentinelApiHandlerMapping extends AbstractHandlerMapping implements EnvironmentAware {
    ...
    @Override
    public void setEnvironment(Environment environment) {
        if (TransportConfig.getPort() == null) {
            TransportConfig.setRuntimePort(environment.getProperty("local.server.port"));
        }
    }
    ...
}
.setParameter("port", TransportConfig.getPort())

This have a problem, [environment.getProperty("local.server.port")] return is null, because setEnvironment is before than ServerPortInfoApplicationContextInitializer set the value. So I want to use another solution, the code is

public class SentinelApiHandlerMapping extends AbstractHandlerMapping implements EnvironmentAware {
    ...
    private static final String SPRING_BOOT_WEB_SERVER_INITIALIZED_EVENT_CLASS = "org.springframework.boot.web.context.WebServerInitializedEvent";
    private static Class webServerInitializedEventClass;
    static {
        try {
            webServerInitializedEventClass = ClassUtils.forName(SPRING_BOOT_WEB_SERVER_INITIALIZED_EVENT_CLASS, null);
            RecordLog.info("[SentinelApiHandlerMapping] class {} is present, this is a spring-boot app, we can auto detect port", SPRING_BOOT_WEB_SERVER_INITIALIZED_EVENT_CLASS);
        } catch (ClassNotFoundException e) {
            RecordLog.info("[SentinelApiHandlerMapping] class {} is not present, this is not a spring-boot app, we can not auto detect port", SPRING_BOOT_WEB_SERVER_INITIALIZED_EVENT_CLASS);
        }
    }
    ...
    @EventListener(ApplicationEvent.class)
    public void detectWebPort(ApplicationEvent applicationEvent) {
        if(webServerInitializedEventClass != null && webServerInitializedEventClass.isAssignableFrom(applicationEvent.getClass())){
            Integer port = null;
            try {
                BeanWrapper beanWrapper = new BeanWrapperImpl(applicationEvent);
                port = (Integer) beanWrapper.getPropertyValue("webServer.port");
            }catch (Exception e){
                RecordLog.warn("[SentinelApiHandlerMapping] resolve port from event " + applicationEvent +  " fail", e);
            }
            if(port != null && TransportConfig.getPort() == null){
                RecordLog.info("[SentinelApiHandlerMapping] resolve port {} from event {}", port, applicationEvent);
                TransportConfig.setRuntimePort(port);
            }
        }
    }
    ...
}
.setParameter("port", TransportConfig.getPort())

how about this way

[cdfive]

@shenbaoyong 你看我最开始发的那个可行么，代码比较少。

@Component
public class SpringMvcHttpHeartbeatSender implements HeartbeatSender, ApplicationListener<WebServerInitializedEvent> {
...
@Override
public void onApplicationEvent(WebServerInitializedEvent event) {
    int port = event.getWebServer().getPort();
    RecordLog.info("[SpringMvcHttpHeartbeatSender]get the web port: {}", port);
    TransportConfig.setRuntimePort(port);
}

新的那个看着步骤稍微有点复杂，本机测过没问题的话就按新的提吧，殊途同归哈: )

[shenbaoyong]

@cdfive 我本机测试了，没有问题，我按照我最新的那种方式提交了。
我这个是参考你的那个简化版来的，我的这个模块没有引入spring-boot的依赖，所以如果直接监听WebServerInitializedEvent的话，编译不通过，所以我用了反射机制来判断WebServerInitializedEvent是否存在，进而知道当前环境是spring-boot还是单纯的spring-mvc。 （你那儿有什么其他的办法能在不引入spring-boot的依赖的情况下能监听WebServerInitializedEvent吗）

[cdfive]

我的这个模块没有引入spring-boot的依赖，所以如果直接监听WebServerInitializedEvent的话，编译不通过，所以我用了反射机制来判断WebServerInitializedEvent是否存在，进而知道当前环境是spring-boot还是单纯的spring-mvc

我觉得你这种实现方式比我之前建议的那种更好，这样不用依赖spring-boot，并且能自动区分是spring-boot还是单纯的spring-mvc。demo本机运行了没问题。通过反射判断可行，我没有找到其他方法，也想的是通过这个。

最后一个小建议是TransportSpringMvcDemoApplication在启动时触发下sentienl初始化，像DashboardApplication里的triggerSentinelInit那样，这样可能会对新用户更友好，配置了连控制台，启动应用即可连上。

在启动TransportSpringMvcDemoApplication后，直接访问
http://locahost:10000/version
或http://locahost:10000/getRules?type=flow
会404，因为CommandCenterInitFunc此时还未初始化，commandCenter没有start()。
需要先访问http://localhost:10000/hello

还有SentinelApiHandlerMapping类107行，}catch (Exception e){这行格式化少了个空格哈：）

[cdfive]

It works well, I've no futher idea.
My network is very slow to open the CI build page.
Could you please help solve the CI build error and approve this PR? @sczyh30 @jasonjoo2010 @linlinisme

[jasonjoo2010]

Nice work, Baoyong!
I dropped some comments and please have a look when you have time.

Thanks.

[sczyh30]

@shenbaoyong Nice work. As the CI indicated, you may need to polish your command center implementation with the nouveau SPI mechanism (introduced in #1383)

[shenbaoyong]

@shenbaoyong Nice work. As the CI indicated, you may need to polish your command center implementation with the nouveau SPI mechanism (introduced in #1383)

@jasonjoo2010
这个当时是continuous-integration/travis-ci/pr 通不过，我按照提示改好了。
现在 integration-test 报错，ParamFlowDefaultCheckerTest.testParamFlowDefaultCheckSingleValueCheckQpsMultipleThreads这个测试用例没通过。 我在本地把sentinel-parameter-flow-control这个模块的测试用例跑了很多遍，大部分情况下都会成功，有两个测试用例偶尔会报错，日志如下，请帮忙看下

Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 4.705 s <<< FAILURE! - in com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowDefaultCheckerTest
testParamFlowDefaultCheckSingleValueCheckQpsMultipleThreads(com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowDefaultCheckerTest)  Time elapsed: 4.077 s  <<< FAILURE!
ParamFlowDefaultCheckerTest.testParamFlowDefaultCheckSingleValueCheckQpsMultipleThreads:323 expected:<6> but was:<5>


Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 9.007 s <<< FAILURE! - in com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowThrottleRateLimitingCheckerTest
testSingleValueThrottleCheckQpsMultipleThreads(com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowThrottleRateLimitingCheckerTest)  Time elapsed: 4.025 s  <<< FAILURE!
ParamFlowThrottleRateLimitingCheckerTest.testSingleValueThrottleCheckQpsMultipleThreads:167 expected:<6> but was:<5>

[jasonjoo2010]

I have no further concerns based on the previous review.
So LGTM now.

But there are so many changes introduced since the last review I don't know what happened. Is there any changes on other branches which have not merged yet that was merged into this PR?

[jasonjoo2010]

@shenbaoyong Nice work. As the CI indicated, you may need to polish your command center implementation with the nouveau SPI mechanism (introduced in #1383)

@jasonjoo2010
这个当时是continuous-integration/travis-ci/pr 通不过，我按照提示改好了。
现在 integration-test 报错，ParamFlowDefaultCheckerTest.testParamFlowDefaultCheckSingleValueCheckQpsMultipleThreads这个测试用例没通过。 我在本地把sentinel-parameter-flow-control这个模块的测试用例跑了很多遍，大部分情况下都会成功，有两个测试用例偶尔会报错，日志如下，请帮忙看下

Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 4.705 s <<< FAILURE! - in com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowDefaultCheckerTest
testParamFlowDefaultCheckSingleValueCheckQpsMultipleThreads(com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowDefaultCheckerTest)  Time elapsed: 4.077 s  <<< FAILURE!
ParamFlowDefaultCheckerTest.testParamFlowDefaultCheckSingleValueCheckQpsMultipleThreads:323 expected:<6> but was:<5>


Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 9.007 s <<< FAILURE! - in com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowThrottleRateLimitingCheckerTest
testSingleValueThrottleCheckQpsMultipleThreads(com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowThrottleRateLimitingCheckerTest)  Time elapsed: 4.025 s  <<< FAILURE!
ParamFlowThrottleRateLimitingCheckerTest.testSingleValueThrottleCheckQpsMultipleThreads:167 expected:<6> but was:<5>

There are absolutely some tests that are not well designed like simulating the racing conditions that are not stable. So just ignore them or if you can improve one or more of them you could submit another pull request kindly.

[sczyh30]

Nice work. Thanks for contributing and look forward to more!

[sczyh30]

@shenbaoyong Nice work. As the CI indicated, you may need to polish your command center implementation with the nouveau SPI mechanism (introduced in #1383)

@jasonjoo2010
这个当时是continuous-integration/travis-ci/pr 通不过，我按照提示改好了。
现在 integration-test 报错，ParamFlowDefaultCheckerTest.testParamFlowDefaultCheckSingleValueCheckQpsMultipleThreads这个测试用例没通过。 我在本地把sentinel-parameter-flow-control这个模块的测试用例跑了很多遍，大部分情况下都会成功，有两个测试用例偶尔会报错，日志如下，请帮忙看下

Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 4.705 s <<< FAILURE! - in com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowDefaultCheckerTest
testParamFlowDefaultCheckSingleValueCheckQpsMultipleThreads(com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowDefaultCheckerTest)  Time elapsed: 4.077 s  <<< FAILURE!
ParamFlowDefaultCheckerTest.testParamFlowDefaultCheckSingleValueCheckQpsMultipleThreads:323 expected:<6> but was:<5>


Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 9.007 s <<< FAILURE! - in com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowThrottleRateLimitingCheckerTest
testSingleValueThrottleCheckQpsMultipleThreads(com.alibaba.csp.sentinel.slots.block.flow.param.ParamFlowThrottleRateLimitingCheckerTest)  Time elapsed: 4.025 s  <<< FAILURE!
ParamFlowThrottleRateLimitingCheckerTest.testSingleValueThrottleCheckQpsMultipleThreads:167 expected:<6> but was:<5>

There are absolutely some tests that are not well designed like simulating the racing conditions that are not stable. So just ignore them or if you can improve one or more of them you could submit another pull request kindly.

Refer #1752