You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/07/23 01:15:31 UTC
[GitHub] [incubator-apisix] nic-chen opened a new pull request #1888: change: add more prometheus metrics(hostname, etcd modify index)
nic-chen opened a new pull request #1888:
URL: https://github.com/apache/incubator-apisix/pull/1888
### What this PR does / why we need it:
add more prometheus metrics for better understanding the situation of APISIX nodes.
#1885
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on pull request #1888: change: add more prometheus metrics(etcd modify index)
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1888:
URL: https://github.com/apache/incubator-apisix/pull/1888#issuecomment-666129858
@nic-chen please update the checklist, it seems that we are missing `Have you modified the corresponding document?` .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on pull request #1888: change: add more prometheus metrics(etcd modify index)
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1888:
URL: https://github.com/apache/incubator-apisix/pull/1888#issuecomment-663918587
Because we added some new metrics this time, it will affect the performance, so let’s do a simple performance test.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on a change in pull request #1888: change: add more prometheus metrics(etcd modify index)
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1888:
URL: https://github.com/apache/incubator-apisix/pull/1888#discussion_r462603072
##########
File path: apisix/core/config_etcd.lua
##########
@@ -201,10 +221,24 @@ local function sync_data(self)
return true
end
- local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1)
+ -- for fetch the etcd index
+ local key_res, _ = getkey(self.etcd_cli, self.key)
Review comment:
we need to capture the `err` message and check it first
##########
File path: apisix/core/config_etcd.lua
##########
@@ -228,7 +228,7 @@ local function sync_data(self)
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
- if not dir_res then
+ if err and err == "timeout" then
Review comment:
`if err == "timeout" then` is enough
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on a change in pull request #1888: change: add more prometheus metrics(etcd modify index)
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1888:
URL: https://github.com/apache/incubator-apisix/pull/1888#discussion_r460458881
##########
File path: apisix/plugins/prometheus/exporter.lua
##########
@@ -152,9 +165,70 @@ local function nginx_status()
label_values[1] = name
metrics.connections:set(val[0], label_values)
+
end
end
+local key_values = {}
+local function set_modify_index(key, items, items_ver, global_max_index)
+ local max_idx = 0
Review comment:
we'd better clear the `key_values`, it is safer.
##########
File path: apisix/plugins/prometheus/exporter.lua
##########
@@ -152,9 +165,70 @@ local function nginx_status()
label_values[1] = name
metrics.connections:set(val[0], label_values)
+
end
end
+local key_values = {}
+local function set_modify_index(key, items, items_ver, global_max_index)
+ local max_idx = 0
+
+ if items_ver and items then
+ for _, item in ipairs(items) do
+ if type(item) == "table" and item.modifiedIndex > max_idx then
+ max_idx = item.modifiedIndex
+ end
+ end
+ end
+
+ key_values[1] = key
+ metrics.etcd_modify_indexes:set(max_idx, key_values)
+
+
+ global_max_index = max_idx > global_max_index and max_idx or global_max_index
+
+ return global_max_index
+end
+local function etcd_modify_index()
Review comment:
code style: two blank lines between different functions.
##########
File path: apisix/plugins/prometheus/exporter.lua
##########
@@ -152,9 +165,70 @@ local function nginx_status()
label_values[1] = name
metrics.connections:set(val[0], label_values)
+
end
end
+local key_values = {}
+local function set_modify_index(key, items, items_ver, global_max_index)
+ local max_idx = 0
+
+ if items_ver and items then
+ for _, item in ipairs(items) do
+ if type(item) == "table" and item.modifiedIndex > max_idx then
+ max_idx = item.modifiedIndex
+ end
+ end
+ end
+
+ key_values[1] = key
+ metrics.etcd_modify_indexes:set(max_idx, key_values)
+
+
+ global_max_index = max_idx > global_max_index and max_idx or global_max_index
+
+ return global_max_index
+end
+local function etcd_modify_index()
+ local global_max_idx = 0
+
+ -- routes
+ local routes, routes_ver = get_routes()
+ global_max_idx = set_modify_index("routes", routes, routes_ver, global_max_idx)
+
+ -- services
+ local services, services_ver = get_services()
+ global_max_idx = set_modify_index("services", services, services_ver, global_max_idx)
+
+ -- ssls
+ local ssls, ssls_ver = get_ssls()
+ global_max_idx = set_modify_index("ssls", ssls, ssls_ver, global_max_idx)
+
+ -- consumers
+ local consumers, consumers_ver = get_consumers()
+ global_max_idx = set_modify_index("consumers", consumers, consumers_ver, global_max_idx)
+
+ -- consumers
+ local consumers, consumers_ver = get_consumers()
+ global_max_idx = set_modify_index("consumers", consumers, consumers_ver, global_max_idx)
+
+ -- global_rules
+ local global_rules = router.global_rules
+ if global_rules then
+ global_max_idx = set_modify_index("global_rules", global_rules.values,
+ global_rules.conf_version, global_max_idx)
+ end
+
+ -- upstreams
+ local upstreams, upstreams_ver = get_upstreams()
+ global_max_idx = set_modify_index("upstreams", upstreams, upstreams_ver, global_max_idx)
+
+ -- global max
+ key_values[1] = "max_modify_index"
Review comment:
clear table `key_values` too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] nic-chen commented on a change in pull request #1888: change: add more prometheus metrics(etcd modify index)
Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #1888:
URL: https://github.com/apache/incubator-apisix/pull/1888#discussion_r462343820
##########
File path: apisix/core/config_etcd.lua
##########
@@ -221,10 +221,22 @@ local function sync_data(self)
return true
end
- local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1)
+ local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1, self.timeout)
+
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
if not dir_res then
+ -- for fetch the last etcd index
+ local key_res, _ = getkey(self.etcd_cli, self.key)
+ if key_res and key_res.headers then
+ local key_index = key_res.headers["X-Etcd-Index"]
+ local key_idx = key_index and tonumber(key_index) or 0
+ if key_idx and key_idx > self.prev_index then
+ -- Avoid the index to exceed 1000 by updating other keys that will causing a full reload
Review comment:
I have split that into two lines.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] moonming commented on a change in pull request #1888: change: add more prometheus metrics(etcd modify index)
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #1888:
URL: https://github.com/apache/incubator-apisix/pull/1888#discussion_r459811000
##########
File path: apisix/plugins/prometheus/exporter.lua
##########
@@ -152,9 +165,70 @@ local function nginx_status()
label_values[1] = name
metrics.connections:set(val[0], label_values)
+
end
end
+local key_values = {}
+local function set_modify_index(key, items, items_ver, global_max_index)
+ local max_idx = 0
+
+ if items_ver and items then
+ for _, item in ipairs(items) do
+ if type(item) == "table" and item.modifiedIndex > max_idx then
+ max_idx = item.modifiedIndex
+ end
+ end
+ end
+
+ key_values[1] = key
+ metrics.etcd_modify_indexes:set(max_idx, key_values)
+
+
+ global_max_index = max_idx > global_max_index and max_idx or global_max_index
+
+ return global_max_index
+end
+local function etcd_modify_index()
Review comment:
need test cases for this function
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on pull request #1888: change: add more prometheus metrics(etcd modify index)
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1888:
URL: https://github.com/apache/incubator-apisix/pull/1888#issuecomment-665122748
@nic-chen if you have fixed them, you can click this button, it means you have fixed it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on a change in pull request #1888: change: add more prometheus metrics(etcd modify index)
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1888:
URL: https://github.com/apache/incubator-apisix/pull/1888#discussion_r462229428
##########
File path: apisix/core/config_etcd.lua
##########
@@ -221,10 +221,22 @@ local function sync_data(self)
return true
end
- local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1)
+ local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1, self.timeout)
+
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
if not dir_res then
+ -- for fetch the last etcd index
+ local key_res, _ = getkey(self.etcd_cli, self.key)
+ if key_res and key_res.headers then
+ local key_index = key_res.headers["X-Etcd-Index"]
+ local key_idx = key_index and tonumber(key_index) or 0
+ if key_idx and key_idx > self.prev_index then
+ -- Avoid the index to exceed 1000 by updating other keys that will causing a full reload
Review comment:
this line is too long
##########
File path: apisix/core/config_etcd.lua
##########
@@ -221,10 +221,22 @@ local function sync_data(self)
return true
end
- local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1)
+ local dir_res, err = waitdir(self.etcd_cli, self.key, self.prev_index + 1, self.timeout)
+
log.info("waitdir key: ", self.key, " prev_index: ", self.prev_index + 1)
log.info("res: ", json.delay_encode(dir_res, true))
if not dir_res then
+ -- for fetch the last etcd index
Review comment:
that is wrong. we need to fetch the last etcd index before `local dir_res, err = waitdir(... ...`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org