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 2022/01/10 03:05:28 UTC
[GitHub] [apisix] zhendongcmss opened a new pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
zhendongcmss opened a new pull request #6055:
URL: https://github.com/apache/apisix/pull/6055
fix https://github.com/apache/apisix/issues/6011
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
<!--
Please follow the PR manners:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. If you need to resolve merge conflicts after the PR is reviewed, please merge master but do not rebase
6. Use "request review" to notify the reviewer once you have resolved the review
7. Only reviewer can click "Resolve conversation" to mark the reviewer's review resolved
-->
* [ ] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r780914424
##########
File path: t/plugin/prometheus3.t
##########
@@ -90,3 +90,106 @@ passed
[200, 200]
--- no_error_log
[error]
+
+
+
+=== TEST 3: apisix_batch_process_entries, mess with global rules
Review comment:
OK
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r780921028
##########
File path: apisix/utils/batch-processor.lua
##########
@@ -161,7 +161,7 @@ function batch_processor:push(entry)
local entries = self.entry_buffer.entries
table.insert(entries, entry)
-- add batch metric for every route
- if batch_metrics then
+ if batch_metrics and self.name and self.route_id and self.server_addr then
Review comment:
Do you mean provider `''` for route_id on
https://github.com/apache/apisix/blob/ddb9cd28bf33799652d252df0546aa0832933bcf/apisix/utils/batch-processor.lua#L138
We can do it, but I think it doesn't change the conditional judgment here. How about you ?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r781686831
##########
File path: apisix/utils/batch-processor.lua
##########
@@ -161,7 +161,7 @@ function batch_processor:push(entry)
local entries = self.entry_buffer.entries
table.insert(entries, entry)
-- add batch metric for every route
- if batch_metrics then
+ if batch_metrics and self.name and self.route_id and self.server_addr then
Review comment:
Yes. So that we can remove repeated conditional judgment.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#issuecomment-1010732326
@tzssangglass Pls help 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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r782659866
##########
File path: apisix/utils/batch-processor.lua
##########
@@ -161,7 +161,7 @@ function batch_processor:push(entry)
local entries = self.entry_buffer.entries
table.insert(entries, entry)
-- add batch metric for every route
- if batch_metrics then
+ if batch_metrics and self.name and self.route_id and self.server_addr then
Review comment:
okay
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r781796409
##########
File path: apisix/utils/batch-processor.lua
##########
@@ -161,7 +161,7 @@ function batch_processor:push(entry)
local entries = self.entry_buffer.entries
table.insert(entries, entry)
-- add batch metric for every route
- if batch_metrics then
+ if batch_metrics and self.name and self.route_id and self.server_addr then
Review comment:
We can use `ctx.var.server_addr` for 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r781796409
##########
File path: apisix/utils/batch-processor.lua
##########
@@ -161,7 +161,7 @@ function batch_processor:push(entry)
local entries = self.entry_buffer.entries
table.insert(entries, entry)
-- add batch metric for every route
- if batch_metrics then
+ if batch_metrics and self.name and self.route_id and self.server_addr then
Review comment:
We can use `ngx.var.server_addr` for 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r782638309
##########
File path: apisix/utils/batch-processor.lua
##########
@@ -161,7 +161,7 @@ function batch_processor:push(entry)
local entries = self.entry_buffer.entries
table.insert(entries, entry)
-- add batch metric for every route
- if batch_metrics then
+ if batch_metrics and self.name and self.route_id and self.server_addr then
Review comment:
I see. We can't use `ngx.var` in the timer. What about refactoring `self.name and self.route_id and self.server_addr` into a function? Would be better if we don't need to repeat the complex conditions.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r782199091
##########
File path: apisix/utils/batch-processor.lua
##########
@@ -161,7 +161,7 @@ function batch_processor:push(entry)
local entries = self.entry_buffer.entries
table.insert(entries, entry)
-- add batch metric for every route
- if batch_metrics then
+ if batch_metrics and self.name and self.route_id and self.server_addr then
Review comment:
I add two line `local ngx = ngx` and `core.log.error("ngx.var.server_addr",ngx.var.server_addr)` on batch-processor.lua but got an error. It seems it deson't use `ngx` on log phase.
```
2022/01/11 22:23:46 [error] 21652#21652: *217 lua user thread aborted: runtime error: /usr/local/apisix/apisix/utils/batch-processor.lua:138: API disabled in the current context
stack traceback:
coroutine 0:
[C]: in function 'error'
/usr/local/openresty/lualib/resty/core/var.lua:103: in function '__index'
/usr/local/apisix/apisix/utils/batch-processor.lua:138: in function 'new'
/usr/local/apisix/apisix/plugins/error-log-logger.lua:316: in function </usr/local/apisix/apisix/plugins/error-log-logger.lua:250>
coroutine 1:
[C]: in function 'thread_spawn'
/usr/local/apisix/apisix/timers.lua:41: in function </usr/local/apisix/apisix/timers.lua:32>
[C]: in function 'pcall'
/usr/local/apisix/apisix/core/timer.lua:35: in function </usr/local/apisix/apisix/core/timer.lua:31>
[C]: in function 'pcall'
/usr/local/apisix/apisix/core/timer.lua:58: in function </usr/local/apisix/apisix/core/timer.lua:51>, context: ngx.timer
2022/01/11 22:23:46 [error] 21649#21649: *218 lua user thread aborted: runtime error: /usr/local/apisix/apisix/utils/batch-processor.lua:138: API disabled in the current context
stack traceback:
coroutine 0:
[C]: in function 'error'
/usr/local/openresty/lualib/resty/core/var.lua:103: in function '__index'
/usr/local/apisix/apisix/utils/batch-processor.lua:138: in function 'new'
/usr/local/apisix/apisix/plugins/error-log-logger.lua:316: in function </usr/local/apisix/apisix/plugins/error-log-logger.lua:250>
coroutine 1:
[C]: in function 'thread_spawn'
/usr/local/apisix/apisix/timers.lua:41: in function </usr/local/apisix/apisix/timers.lua:32>
[C]: in function 'pcall'
/usr/local/apisix/apisix/core/timer.lua:35: in function </usr/local/apisix/apisix/core/timer.lua:31>
[C]: in function 'pcall'
/usr/local/apisix/apisix/core/timer.lua:58: in function </usr/local/apisix/apisix/core/timer.lua:51>, context: ngx.timer
2022/01/11 22:23:46 [error] 21652#21652: *217 [lua] timers.lua:54: failed to wait threads: API disabled in the current context, context: ngx.timer
2022/01/11 22:23:46 [error] 21649#21649: *218 [lua] timers.lua:54: failed to wait threads: API disabled in the current context, context: ngx.timer
2022/01/11 22:23:46 [error] 21648#21648: *223 lua user thread aborted: runtime error: /usr/local/apisix/apisix/utils/batch-processor.lua:138: API disabled in the current context
stack traceback:
coroutine 0:
[C]: in function 'error'
/usr/local/openresty/lualib/resty/core/var.lua:103: in function '__index'
/usr/local/apisix/apisix/utils/batch-processor.lua:138: in function 'new'
/usr/local/apisix/apisix/plugins/error-log-logger.lua:316: in function </usr/local/apisix/apisix/plugins/error-log-logger.lua:250>
coroutine 1:
[C]: in function 'thread_spawn'
/usr/local/apisix/apisix/timers.lua:41: in function </usr/local/apisix/apisix/timers.lua:32>
[C]: in function 'pcall'
/usr/local/apisix/apisix/core/timer.lua:35: in function </usr/local/apisix/apisix/core/timer.lua:31>
[C]: in function 'pcall'
/usr/local/apisix/apisix/core/timer.lua:58: in function </usr/local/apisix/apisix/core/timer.lua:51>, context: ngx.timer
2022/01/11 22:23:46 [error] 21648#21648: *223 [lua] timers.lua:54: failed to wait threads: API disabled in the current context, context: ngx.timer
```
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r780912270
##########
File path: apisix/utils/batch-processor.lua
##########
@@ -161,7 +161,7 @@ function batch_processor:push(entry)
local entries = self.entry_buffer.entries
table.insert(entries, entry)
-- add batch metric for every route
- if batch_metrics then
+ if batch_metrics and self.name and self.route_id and self.server_addr then
Review comment:
We could provide a default `''` for the route_id, right?
> Sometimes we need to judge whether matching route accroding to toute_id.
There is no need to judge with route_id for `error-log-logger`.
##########
File path: t/plugin/prometheus3.t
##########
@@ -90,3 +90,106 @@ passed
[200, 200]
--- no_error_log
[error]
+
+
+
+=== TEST 3: apisix_batch_process_entries, mess with global rules
Review comment:
Could you use the full file in https://github.com/apache/apisix/pull/6047#issuecomment-1008495244 instead of part of 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #6055:
URL: https://github.com/apache/apisix/pull/6055
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on a change in pull request #6055: fix: the prometheus lables are inconsistent when using the same batch-processor instance on multi plugins
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on a change in pull request #6055:
URL: https://github.com/apache/apisix/pull/6055#discussion_r781694136
##########
File path: apisix/utils/batch-processor.lua
##########
@@ -161,7 +161,7 @@ function batch_processor:push(entry)
local entries = self.entry_buffer.entries
table.insert(entries, entry)
-- add batch metric for every route
- if batch_metrics then
+ if batch_metrics and self.name and self.route_id and self.server_addr then
Review comment:
If removing the repeated conditional judgment, I think should provider a default value `''` for `server_addr`, error-log-logger doesn't have the `server_addr` 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org