You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by GitBox <gi...@apache.org> on 2022/07/24 13:47:03 UTC
[GitHub] [incubator-brpc] jenrryyou opened a new pull request, #1854: support customized server bvar prefix
jenrryyou opened a new pull request, #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854
通过ServerOption自定义Server的Bvar prefix。
目前bvar默认的前缀是rpc_server_{server监听的port}，但是存在如下问题：
1. 由于port改变（如使用range port或者其他port可能会改变的场景)导致bvar name变化，使得配置的dashboard和监控失效
2. 为了支持IPV6和Unix Domain socket，引入了ExtendedEndPoint，不过生成prefix存在BUG，会固定用EXTENDED_ENDPOINT_PORT（123456789）生成前缀（rpc_server_123456789），导致不同的Server bvar前缀相同。虽然修复不难，但是Unix Domain socket不太好处理，因为没有port信息，如果用file_path的话又太长。
基于以上考虑，最好还是用户通过ServerOption传入Server name生成prefix，如果name为空，则保持原来的逻辑。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] serverglen commented on a diff in pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
serverglen commented on code in PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#discussion_r945464629
##########
src/brpc/server.h:
##########
@@ -241,6 +241,10 @@ struct ServerOptions {
// Default: NULL (disabled)
RedisService* redis_service;
+ // Optional name for composing server bvar prefix. Read ServerPrefix() method for details;
+ // Default: ""
+ std::string name;
Review Comment:
server_info_name吧
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] wwbmmm commented on pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
wwbmmm commented on PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#issuecomment-1200668513
>
> 用gflag如果有多个server的情况不能区分各自server的指标，个人觉得这类server spefic的属性放server option更合理一些。
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jenrryyou commented on pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#issuecomment-1193495911
> 百度内部是通过gflag来自定义server prefix的，代码大致如下：
>
> ```c++
> std::string Server::ServerPrefix() const {
> if (FLAGS_customized_server_prefix.empty()) {
> return base::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
> } else {
> return FLAGS_customized_server_prefix;
> }
> }
> ```
>
> 可以看下哪种方式更方便一些。
用gflag的话如果有多个server的话不能区分各自server的指标，我觉得这类server spefic的属性放server option更合理一些
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jenrryyou commented on a diff in pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on code in PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#discussion_r943204208
##########
src/brpc/server.h:
##########
@@ -241,6 +241,10 @@ struct ServerOptions {
// Default: NULL (disabled)
RedisService* redis_service;
+ // Optional name for composing server bvar prefix. Read ServerPrefix() method for details;
+ // Default: ""
+ std::string name;
Review Comment:
> 感觉name有点通用，要不换个名字？比如：server_info_name，或者server_bvar_name。
本意是指用户传入的Server唯一标识(取代Port这种网络领域的标识)。不过name确实太通用，而且容易和service_name混淆。bvar_name又太具体了，这个标识以后也有其他的作用（例如日志打印）。要不改成customized_tag/customized_label代表用户传入的Server标识？server_info_name也可以，看看你觉得哪个更好。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] wwbmmm commented on pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
wwbmmm commented on PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#issuecomment-1193481790
百度内部是通过gflag来自定义server prefix的，代码大致如下：
```cpp
std::string Server::ServerPrefix() const {
if (FLAGS_customized_server_prefix.empty()) {
return base::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
} else {
return FLAGS_customized_server_prefix;
}
}
```
可以看下哪种方式更方便一些。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] serverglen commented on a diff in pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
serverglen commented on code in PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#discussion_r934200122
##########
src/brpc/server.cpp:
##########
@@ -271,7 +271,11 @@ static bvar::Vector<unsigned, 2> GetSessionLocalDataCount(void* arg) {
}
std::string Server::ServerPrefix() const {
- return butil::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
+ if(_options.name.empty()) {
+ return butil::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
+ } else {
+ return std::string(g_server_info_prefix) + "_" + _options.name;
Review Comment:
改成 return _options.name;是不是更好？
我理解既然要自定义server bvar prefix，就最好全部自定义。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] serverglen merged pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
serverglen merged PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jenrryyou commented on a diff in pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on code in PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#discussion_r939460992
##########
src/brpc/server.cpp:
##########
@@ -271,7 +271,11 @@ static bvar::Vector<unsigned, 2> GetSessionLocalDataCount(void* arg) {
}
std::string Server::ServerPrefix() const {
- return butil::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
+ if(_options.name.empty()) {
+ return butil::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
+ } else {
+ return std::string(g_server_info_prefix) + "_" + _options.name;
Review Comment:
目前PrometheusMetricsDumper::DumpLatencyHistogramRecorderSuffix通过g_server_info_prefix前缀来区分哪些bvar需要dump，全部自定义不好兼容这个逻辑。
```cpp
int DumpPrometheusMetricsToIOBuf(butil::IOBuf* output) {
butil::IOBufBuilder os;
PrometheusMetricsDumper dumper(&os, g_server_info_prefix);
const int ndump = bvar::Variable::dump_exposed(&dumper, NULL);
if (ndump < 0) {
return -1;
}
os.move_to(*output);
return 0;
}
bool PrometheusMetricsDumper::DumpLatencyHistogramRecorderSuffix(
const butil::StringPiece& name,
const butil::StringPiece& desc) {
if (!name.starts_with(_server_prefix)) {
return false;
}
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jenrryyou commented on a diff in pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on code in PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#discussion_r939460992
##########
src/brpc/server.cpp:
##########
@@ -271,7 +271,11 @@ static bvar::Vector<unsigned, 2> GetSessionLocalDataCount(void* arg) {
}
std::string Server::ServerPrefix() const {
- return butil::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
+ if(_options.name.empty()) {
+ return butil::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
+ } else {
+ return std::string(g_server_info_prefix) + "_" + _options.name;
Review Comment:
目前PrometheusMetricsDumper::DumpLatencyHistogramRecorderSuffix通过g_server_info_prefix前缀来区分哪些bvar需要导入，全部自定义不好兼容这个逻辑。
```cpp
bool PrometheusMetricsDumper::DumpLatencyHistogramRecorderSuffix(
const butil::StringPiece& name,
const butil::StringPiece& desc) {
if (!name.starts_with(_server_prefix)) {
return false;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jenrryyou commented on a diff in pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on code in PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#discussion_r943204208
##########
src/brpc/server.h:
##########
@@ -241,6 +241,10 @@ struct ServerOptions {
// Default: NULL (disabled)
RedisService* redis_service;
+ // Optional name for composing server bvar prefix. Read ServerPrefix() method for details;
+ // Default: ""
+ std::string name;
Review Comment:
> 感觉name有点通用，要不换个名字？比如：server_info_name，或者server_bvar_name。
本意是指用户传入的Server唯一标识(取代Port这种网络领域的标识)。不过name确实太通用，而且容易和service_name混淆。bvar_name又太具体了，这个标识以后也有其他的作用（例如日志打印）。要不改成customized_tag/customized_label代表用户传入的Server标识？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jenrryyou commented on a diff in pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on code in PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#discussion_r949751266
##########
src/brpc/server.h:
##########
@@ -241,6 +241,10 @@ struct ServerOptions {
// Default: NULL (disabled)
RedisService* redis_service;
+ // Optional name for composing server bvar prefix. Read ServerPrefix() method for details;
+ // Default: ""
+ std::string name;
Review Comment:
好的，已经改过了，有空辛苦review下~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jenrryyou commented on a diff in pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on code in PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#discussion_r939460992
##########
src/brpc/server.cpp:
##########
@@ -271,7 +271,11 @@ static bvar::Vector<unsigned, 2> GetSessionLocalDataCount(void* arg) {
}
std::string Server::ServerPrefix() const {
- return butil::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
+ if(_options.name.empty()) {
+ return butil::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
+ } else {
+ return std::string(g_server_info_prefix) + "_" + _options.name;
Review Comment:
目前PrometheusMetricsDumper::DumpLatencyHistogramRecorderSuffix通过g_server_info_prefix前缀来区分哪些bvar需要dump，全部自定义不好兼容这个逻辑。
```cpp
bool PrometheusMetricsDumper::DumpLatencyHistogramRecorderSuffix(
const butil::StringPiece& name,
const butil::StringPiece& desc) {
if (!name.starts_with(_server_prefix)) {
return false;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] serverglen commented on a diff in pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
serverglen commented on code in PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#discussion_r939778363
##########
src/brpc/server.h:
##########
@@ -241,6 +241,10 @@ struct ServerOptions {
// Default: NULL (disabled)
RedisService* redis_service;
+ // Optional name for composing server bvar prefix. Read ServerPrefix() method for details;
+ // Default: ""
+ std::string name;
Review Comment:
感觉name有点通用，要不换个名字？比如：server_info_name，或者server_bvar_name。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jenrryyou commented on pull request #1854: support customized server bvar prefix
Posted by GitBox <gi...@apache.org>.
jenrryyou commented on PR #1854:
URL: https://github.com/apache/incubator-brpc/pull/1854#issuecomment-1193496274
> 百度内部是通过gflag来自定义server prefix的，代码大致如下：
>
> ```c++
> std::string Server::ServerPrefix() const {
> if (FLAGS_customized_server_prefix.empty()) {
> return base::string_printf("%s_%d", g_server_info_prefix, listen_address().port);
> } else {
> return FLAGS_customized_server_prefix;
> }
> }
> ```
>
> 可以看下哪种方式更方便一些。
用gflag的话如果有多个server的话不能区分各自server的指标，我觉得这类server spefic的属性放server option更合理一些
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org