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

【腾讯犀牛鸟开源课题实战】prometheus插件专项建设(PUSH模式支持等) #175

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Heaven2024
Copy link

@Heaven2024 Heaven2024 commented Aug 26, 2024

issue
完成了push模式以及promethues的鉴权支持

note:
修正了md文档相关问题
另外prometheus示例文件中的运行脚本无法直接运行,编译后的路径名和config参数不对:
/home/TRPC/trpc-cpp/examples/features/prometheus/run.sh: line 9: ./bazel-bin/examples/features/prometheus/client/client_config: No such file or directory

ERROR: unknown command line flag 'config'

@Heaven2024 Heaven2024 changed the title Fixed prometheus documentation and running scripts 【腾讯犀牛鸟开源课题实战】prometheus插件专项建设(PUSH模式支持等) Oct 7, 2024
@weimch
Copy link
Contributor

weimch commented Oct 11, 2024

这个问题有修复吗?

unknown command line flag 'config'

@@ -3,3 +3,5 @@ build --copt=-O2
#build --copt=-g --strip=never
build --jobs 16
#test --cache_test_results=no --test_output=errors

build --define trpc_include_prometheus=true
Copy link
Contributor

Choose a reason for hiding this comment

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

默认关闭prometheus,这行可以删掉

@Heaven2024
Copy link
Author

这个问题有修复吗?

unknown command line flag 'config'

fixed

@@ -0,0 +1,22 @@
#include <chrono>
Copy link
Contributor

Choose a reason for hiding this comment

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

这个文件和框架无关,没必要增加,用法放在文档就好了

@@ -77,6 +77,12 @@ ::trpc::Status ForwardServiceImpl::Route(::trpc::ServerContextPtr context,
"counter_name", "counter_desc", {{"const_counter_key", "const_counter_value"}});
::prometheus::Counter& counter = counter_family->Add({{"counter_key", "counter_value"}});
counter.Increment(random_num);

if (::trpc::prometheus::PushMetricsInfo()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

为啥这里还需要手动调用呢?不能配置一下yaml文件就生效吗?

@@ -0,0 +1,78 @@
<div id="layer1">
Copy link
Contributor

Choose a reason for hiding this comment

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

去掉不需要的文件

Choose a reason for hiding this comment

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

已删除。

@@ -44,3 +44,13 @@ cc_library(
"@trpc_cpp//trpc/metrics/prometheus:prometheus_metrics_api",
],
)

cc_binary(
Copy link
Contributor

Choose a reason for hiding this comment

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

不需要push这个文件,去掉与之相关的编译引入

} else {
TRPC_LOG_INFO("can not found prometheus auth config");
has_cfg = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

构造函数做了太复杂的事情,可以定义一个Init函数,把这部分逻辑放在Init函数里

Choose a reason for hiding this comment

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

已修改。


void PrometheusHandler::CommandHandle(http::HttpRequestPtr req, rapidjson::Value& result,
rapidjson::Document::AllocatorType& alloc) {
static std::unique_ptr<::prometheus::Serializer> serializer = std::make_unique<::prometheus::TextSerializer>();

if (has_cfg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

has_cfg变量名不好,看起来只跟鉴权判断相关,同时可以考虑把鉴权相关信息统一放在一个结构体里。

Choose a reason for hiding this comment

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

已修改。


void PrometheusHandler::CommandHandle(http::HttpRequestPtr req, rapidjson::Value& result,
rapidjson::Document::AllocatorType& alloc) {
static std::unique_ptr<::prometheus::Serializer> serializer = std::make_unique<::prometheus::TextSerializer>();

if (has_cfg) {
std::string token = req->GetHeader("Authorization");
Copy link
Contributor

Choose a reason for hiding this comment

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

这块逻辑有啥用?看起来只是判断用户名和密码是否匹配,判断之后有啥用?

Choose a reason for hiding this comment

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

只有用户名密码都正确的情况下,才会返回metric数据,否则拒绝请求。

Copy link
Contributor

@weimch weimch Oct 15, 2024

Choose a reason for hiding this comment

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

用户名和密码,在prometheus的gateway服务哪里能配置呢?文档有给出吗?

Copy link
Contributor

Choose a reason for hiding this comment

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

想起来了,这是pull模式的,那用户名和密码在prometheus服务器里哪里能配置呢?

Choose a reason for hiding this comment

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

文档我还没有写,确认一下,Prometheus鉴权相关的使用方法是直接添加在prometheus_metrics.md吗?

@@ -26,6 +33,12 @@ class PrometheusHandler : public AdminHandlerBase {

void CommandHandle(http::HttpRequestPtr req, rapidjson::Value& result,
rapidjson::Document::AllocatorType& alloc) override;
private:
PrometheusConfig prometheus_conf_;
Copy link
Contributor

Choose a reason for hiding this comment

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

还是不理解鉴权相关的参数放admin服务的意图,可以描述下

Choose a reason for hiding this comment

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

因为Prometheus拉取数据是要走admin服务的,我感觉只有在这里才能拿到http包头中的用户名密码信息,才能进行鉴权。

Copy link
Contributor

Choose a reason for hiding this comment

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

看实现,只需要填充username和password就好了吧?不需要保留prometheus_conf_,只需要填充CommandHandle里的username和password字段

Choose a reason for hiding this comment

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

已修改。

@@ -71,7 +71,7 @@ class Plugin : public RefCounted<Plugin> {

/// @brief Stop the runtime environment of the plugin
virtual void Stop() noexcept {}

Copy link
Contributor

Choose a reason for hiding this comment

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

记得用clang-format把所有代码文件都格式化一遍(使用项目根目录的.clang-format配置的格式化规范)

@@ -71,7 +71,7 @@ class Plugin : public RefCounted<Plugin> {

/// @brief Stop the runtime environment of the plugin
virtual void Stop() noexcept {}

Copy link
Contributor

Choose a reason for hiding this comment

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

这里出现了不必要的空格

bool enabled = false;
std::string gateway_url;
std::string job_name;
int push_interval_seconds = 15;
Copy link
Contributor

Choose a reason for hiding this comment

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

命名为 push_interval,单位为毫秒吧

struct PushMode {
bool enabled = false;
std::string gateway_url;
std::string job_name;
Copy link
Contributor

Choose a reason for hiding this comment

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

job_name这个字段有什么用?

Copy link

github-actions bot commented Oct 12, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@@ -44,6 +44,11 @@ plugins:
const_labels:
const_key1: const_value1
const_key2: const_value2
push_mode:
Copy link
Contributor

Choose a reason for hiding this comment

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

例子需要演示pull模式和push模式,应该给出2个文件配置

"Failed to obtain Prometheus plugin configuration from the framework configuration file. Default configuration "
"will be used.");
}
Init();
Copy link
Contributor

Choose a reason for hiding this comment

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

构造函数别执行太复杂的事情,Init不能放在外面执行吗?

Choose a reason for hiding this comment

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

已修改。


void PrometheusHandler::CommandHandle(http::HttpRequestPtr req, rapidjson::Value& result,
rapidjson::Document::AllocatorType& alloc) {
static std::unique_ptr<::prometheus::Serializer> serializer = std::make_unique<::prometheus::TextSerializer>();

if (auth_conf_.username.size() && auth_conf_.password.size()) {
std::string token = req->GetHeader("Authorization");
auto splited = Split(token, ' ');
Copy link
Contributor

Choose a reason for hiding this comment

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

鉴权部分单独提出一个类私有成员接口

Choose a reason for hiding this comment

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

已修改。

auto sp = Split(username_pwd, ':');
if (sp.size() != 2) {
result.AddMember("message", "wrong request without authorization", alloc);
TRPC_LOG_INFO("error token: " << token);
Copy link
Contributor

Choose a reason for hiding this comment

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

使用错误日志宏 TRPC_FMT_ERROR

Choose a reason for hiding this comment

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

已修改。

}

auto username = sp[0];
if (username != auth_conf_.username) {
Copy link
Contributor

Choose a reason for hiding this comment

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

用户名和密码应该一起判断,如果不对,统一返回 wrong username or password 的信息,现在这种实现,攻击者能猜对用户名

Choose a reason for hiding this comment

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

已修改。

@@ -16,6 +16,12 @@

#include "trpc/admin/admin_handler.h"
#include "trpc/util/prometheus.h"
#include "trpc/util/http/base64.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

用clang-format格式化一下,头文件顺序需要按照字母序顺序排列

Choose a reason for hiding this comment

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

已修改。

PrometheusConfig prometheus_conf_;

struct AuthConf {
std::string username;
Copy link
Contributor

Choose a reason for hiding this comment

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

为啥不使用token的方式来鉴权呢?

Copy link

@leolin49 leolin49 Oct 16, 2024

Choose a reason for hiding this comment

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

是可以使用token,一开始的实现也是token,但是我查了资料,pushgateway没办法用token来鉴权。所以如果pull模式用token的话,就相当于是两套鉴权模式了。

Copy link
Contributor

Choose a reason for hiding this comment

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

push和pull确实是两套鉴权模式吧,配置区分开就好

@@ -73,15 +83,20 @@ cc_library(
":prometheus_conf",
":prometheus_conf_parser",
"//trpc/util:prometheus",
"@com_github_jupp0r_prometheus_cpp//core",

Copy link
Contributor

Choose a reason for hiding this comment

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

不必要的换行,BUILD文件使用 buildifier 格式化一下


namespace trpc {

// PrometheusMetrics::~PrometheusMetrics() {
Copy link
Contributor

Choose a reason for hiding this comment

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

去掉无用注释

bool Push(const std::vector<::prometheus::MetricFamily>& metrics);
bool AsyncPush(const std::vector<::prometheus::MetricFamily>& metrics);

void registerCollectable(const std::weak_ptr<::prometheus::Collectable>& collectable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

接口命名风格,方法名首字母大写,驼峰式命名


PrometheusPusher::PrometheusPusher(const std::string& gateway_url, const std::string& job_name)
: job_name_(job_name) {
gateway_ = std::make_unique<::prometheus::Gateway>("localhost","9091", job_name);
Copy link
Contributor

@weimch weimch Oct 15, 2024

Choose a reason for hiding this comment

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

这里没有使用配置

bool PrometheusPusher::Push(const std::vector<::prometheus::MetricFamily>& metrics)
{
int push_result = gateway_->Push();
if (push_result != 200) {
Copy link
Contributor

Choose a reason for hiding this comment

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

魔数,应该使用Prometheus里专门的错误码

if (push_result != 200) {
std::ostringstream oss;
oss << "Failed to push metrics to Pushgateway. Error code: " << push_result;
TRPC_LOG_ERROR(oss.str());
Copy link
Contributor

Choose a reason for hiding this comment

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

这样写不行吗?

TRPC_LOG_ERROR("Failed to push metrics to Pushgateway. Error code: " << push_result);


// 等待异步操作完成并获取结果
try {
int push_result = push_future.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

这里不是阻塞等吗?从效果上和同步调用有啥区别,不应该用future-then的回调吗

@@ -80,6 +80,19 @@ def trpc_workspace(path_prefix = "", repo_name = "", **kwargs):
urls = zlib_urls,
)


http_archive(
name = "com_github_curl",
Copy link
Contributor

Choose a reason for hiding this comment

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

用 bazel 的 buildifier 工具格式化一下

@@ -80,6 +80,19 @@ def trpc_workspace(path_prefix = "", repo_name = "", **kwargs):
urls = zlib_urls,
)


http_archive(
name = "com_github_curl",
Copy link
Contributor

Choose a reason for hiding this comment

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

这里为啥要引入curl?

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