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/04/28 09:30:20 UTC
[GitHub] [apisix] tzssangglass opened a new pull request, #6960: feat(xRPC): support log filter
tzssangglass opened a new pull request, #6960:
URL: https://github.com/apache/apisix/pull/6960
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r869892506
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -71,9 +79,58 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger then
+ return false
+ end
+
+ if not logger.filter or #logger.filter == 0 then
+ -- no valid filter, default execution plugin
+ return true
+ end
+
+ -- key and version are divided by per-logger level
+ local key = "xrpc-logger" .. ctx.conf_id
Review Comment:
update
--
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 diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r868702309
##########
apisix/stream/xrpc/sdk.lua:
##########
@@ -108,6 +108,10 @@ function _M.get_req_ctx(session, id)
local ctx = core.tablepool.fetch("xrpc_ctxs", 4, 4)
-- fields start with '_' should not be accessed by the protocol implementation
ctx._id = id
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(session._route.protocol)
Review Comment:
Better to set a per-logger conf_id because multiple instances of the same logger plugin can be used together.
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,9 +74,49 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger or not logger.filter or #logger.filter == 0 then
Review Comment:
The logger can't be nil, and we should return true when filter is missing?
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r867301191
##########
apisix/schema_def.lua:
##########
@@ -807,6 +807,26 @@ local xrpc_protocol_schema = {
description = "protocol-specific configuration",
type = "object",
},
+ logger = {
Review Comment:
update
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
+ local logger = session._route.protocol.logger
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(session, ctx)
+ local logger = session._route.protocol.logger
+
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
+
+ if not ok then
Review Comment:
update
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r868828797
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,9 +74,49 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
Review Comment:
update
##########
apisix/stream/xrpc/sdk.lua:
##########
@@ -108,6 +108,10 @@ function _M.get_req_ctx(session, id)
local ctx = core.tablepool.fetch("xrpc_ctxs", 4, 4)
-- fields start with '_' should not be accessed by the protocol implementation
ctx._id = id
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(session._route.protocol)
Review Comment:
update
--
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 diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r870061320
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -90,7 +90,7 @@ local function filter_logger(ctx, logger)
end
-- key and version are divided by per-logger level
- local key = "xrpc-logger" .. ctx.conf_id
+ local key = tostring(ctx.conf_id)
Review Comment:
We can save `tostring` here as `ctx.conf_id` is already a string?
--
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 diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r867484185
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,9 +75,53 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(ctx, logger)
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+ if not ok then
+ core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
+ return
+ end
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
Review Comment:
Can we run these once instead of per-logger?
##########
apisix/schema_def.lua:
##########
@@ -807,6 +807,28 @@ local xrpc_protocol_schema = {
description = "protocol-specific configuration",
type = "object",
},
+ logger = {
+ type = "array",
+ items = {
+ properties = {
+ name = {
+ type = "string",
+ },
+ filter = {
+ description = "log filter rules",
Review Comment:
```suggestion
description = "logger filter rules",
```
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
+ local logger = session._route.protocol.logger
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(session, ctx)
+ local logger = session._route.protocol.logger
+
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
+
+ if not ok then
+ core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
Review Comment:
Missing this one?
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r867301227
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
+ local logger = session._route.protocol.logger
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(session, ctx)
+ local logger = session._route.protocol.logger
+
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
+
+ if not ok then
+ core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
Review Comment:
update
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
Review Comment:
update
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
+ local logger = session._route.protocol.logger
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(session, ctx)
+ local logger = session._route.protocol.logger
+
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
+
+ if not ok then
+ core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
+ return
+ end
+
+ local log_func = plugin.log
+ if log_func then
+ log_func(logger.conf, ctx)
+ end
+end
+
+
local function finish_req(protocol, session, ctx)
ctx._rpc_end_time = ngx_now()
- protocol.log(session, ctx)
+ local matched = log_filter(session, ctx)
+ core.log.info("log filter result: ", matched)
+
+ if matched then
+ protocol.log(session, ctx)
Review Comment:
update
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r867596311
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
+ local logger = session._route.protocol.logger
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(session, ctx)
+ local logger = session._route.protocol.logger
+
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
+
+ if not ok then
+ core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
Review Comment:
update
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r868828914
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,9 +74,49 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger or not logger.filter or #logger.filter == 0 then
Review Comment:
update
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r869892336
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -14,14 +14,22 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
+local require = require
local core = require("apisix.core")
+local expr = require("resty.expr.v1")
local pairs = pairs
local ngx = ngx
local ngx_now = ngx.now
local OK = ngx.OK
local DECLINED = ngx.DECLINED
local DONE = ngx.DONE
+local pcall = pcall
+local ipairs = ipairs
+local tostring = tostring
+local logger_expr_cache = core.lrucache.new({
+ ttl = 300, count = 32
Review Comment:
update
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -71,9 +79,58 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger then
+ return false
+ end
+
+ if not logger.filter or #logger.filter == 0 then
+ -- no valid filter, default execution plugin
+ return true
+ end
+
+ -- key and version are divided by per-logger level
+ local key = "xrpc-logger" .. ctx.conf_id
+ local version = tostring(logger.filter)
+ local filter_expr, err = logger_expr_cache(key, version, expr.new, logger.filter)
+ if not filter_expr or err then
Review Comment:
update
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r867596851
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,9 +75,53 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(ctx, logger)
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+ if not ok then
+ core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
+ return
+ end
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
Review Comment:
There is a problem here, since it is possible to run the plugin in `logger`, `metrics` stage, so `conf_id`, `conf_type` can only be set here. We need to find a way to solve this problem.
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r869831020
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -71,9 +79,58 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger then
+ return false
+ end
+
+ if not logger.filter or #logger.filter == 0 then
+ -- no valid filter, default execution plugin
+ return true
+ end
+
+ -- key and version are divided by per-logger level
+ local key = "xrpc-logger" .. ctx.conf_id
+ local version = tostring(logger.filter)
Review Comment:
But the meaning of `version` here should be related to `filter`. If `conf` does not change but `filter` changed, then `version` should change and a new expr should be generated?
However, as currently designed, if the `conf` is changed via the admin api, the `filter` will also change and use the new memory address.
I was trying to make the place more intuitive.
--
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 diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r867668615
##########
t/xrpc/apisix/stream/xrpc/protocols/pingpong/init.lua:
##########
@@ -76,6 +76,12 @@ local function to_int32(p, idx)
end
+local function init_ctx_fileds(ctx)
+ -- init some ctx fields
+ core.ctx.set_vars_meta(ctx)
Review Comment:
Ditto
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -93,11 +93,13 @@ local function run_log_plugin(ctx, logger)
local pkg_name = "apisix.stream.plugins." .. logger.name
local ok, plugin = pcall(require, pkg_name)
if not ok then
- core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
+ core.log.error("failed to load plugin [", logger.name, "] err: ", plugin)
return
end
- core.ctx.set_vars_meta(ctx)
+ -- we choose to initialize conf_id and conf_type here
+ -- because conf here refers specifically to the conf in the log phase,
+ -- to avoid overwriting the conf in other parts of the protocol
ctx.conf_id = tostring(logger.conf)
ctx.conf_type = "xrpc-logger"
Review Comment:
We should add it to https://github.com/apache/apisix/blob/1b5c1900da517bb7d34d4b221aace7242345e78a/apisix/stream/xrpc/sdk.lua#L110
--
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 diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r868722806
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,9 +74,49 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
Review Comment:
Better avoid using the module name as the var name.
And let's introduce a cache to avoid creating new obj per operations in the next PR.
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r870065550
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -90,7 +90,7 @@ local function filter_logger(ctx, logger)
end
-- key and version are divided by per-logger level
- local key = "xrpc-logger" .. ctx.conf_id
+ local key = tostring(ctx.conf_id)
Review Comment:
update
--
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 diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r866669511
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
+ local logger = session._route.protocol.logger
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(session, ctx)
+ local logger = session._route.protocol.logger
+
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
+
+ if not ok then
Review Comment:
Better to check ok just after pcall
##########
apisix/schema_def.lua:
##########
@@ -807,6 +807,26 @@ local xrpc_protocol_schema = {
description = "protocol-specific configuration",
type = "object",
},
+ logger = {
Review Comment:
Better to use array so that we can add multiple logger in the future
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
+ local logger = session._route.protocol.logger
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(session, ctx)
+ local logger = session._route.protocol.logger
+
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
+
+ if not ok then
+ core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
+ return
+ end
+
+ local log_func = plugin.log
+ if log_func then
+ log_func(logger.conf, ctx)
+ end
+end
+
+
local function finish_req(protocol, session, ctx)
ctx._rpc_end_time = ngx_now()
- protocol.log(session, ctx)
+ local matched = log_filter(session, ctx)
+ core.log.info("log filter result: ", matched)
+
+ if matched then
+ protocol.log(session, ctx)
Review Comment:
We need to run `protocol.log` whether the logger is matched or not, otherwise the reuse logic in
https://github.com/apache/apisix/blob/2800a06f00ff9010d893eb8c4b9c94359cc3a96b/apisix/stream/xrpc/protocols/redis/init.lua#L287 won't work.
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
+ local logger = session._route.protocol.logger
+ if not logger or not logger.filter or #logger.filter == 0 then
+ return false
+ end
+
+ local expr, err = expr.new(logger.filter)
+ if err then
+ core.log.error("failed to validate the 'filter' expression: ", err)
+ return false
+ end
+ return expr:eval(ctx)
+end
+
+
+local function run_log_plugin(session, ctx)
+ local logger = session._route.protocol.logger
+
+ local pkg_name = "apisix.stream.plugins." .. logger.name
+ local ok, plugin = pcall(require, pkg_name)
+
+ core.ctx.set_vars_meta(ctx)
+ ctx.conf_id = tostring(logger.conf)
+ ctx.conf_type = "xrpc-logger"
+
+ if not ok then
+ core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
Review Comment:
```suggestion
core.log.error("failed to load plugin [", logger.name, "] err: ", plugin)
```
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -70,10 +74,54 @@ local function put_req_ctx(session, ctx)
end
+local function log_filter(session, ctx)
Review Comment:
```suggestion
local function filter_logger(session, ctx)
```
would be better?
--
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 diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r867484872
##########
t/xrpc/pingpong2.t:
##########
@@ -139,3 +151,548 @@ lua tcp socket send timeout: 60000
stream lua tcp socket read timeout: 60000
--- log_level: debug
--- stream_conf_enable
+
+
+
+=== TEST 3: bad loggger filter
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ {
+ protocol = {
+ name = "pingpong",
+ logger = {
+ {
+ name = "syslog",
+ filter = {
+ {}
+ },
+ conf = {}
+ }
+ }
+ },
+ upstream = {
+ nodes = {
+ ["127.0.0.1:1995"] = 1
+ },
+ type = "roundrobin"
+ }
+ }
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+
+
+
+=== TEST 4: failed to validate the 'filter' expression
+--- request eval
+"POST /t
+" .
+"pp\x02\x00\x00\x00\x00\x00\x00\x03ABC"
+--- stream_conf_enable
+--- error_log
+failed to validate the 'filter' expression: rule too short
+
+
+
+=== TEST 5: set loggger filter(single rule)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ {
+ protocol = {
+ name = "pingpong",
+ logger = {
+ {
+ name = "syslog",
+ filter = {
+ {"len", ">", 10}
+ },
+ conf = {}
+ }
+ }
+ },
+ upstream = {
+ nodes = {
+ ["127.0.0.1:1995"] = 1
+ },
+ type = "roundrobin"
+ }
+ }
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+
+
+
+=== TEST 6: log filter matched successful
+--- request eval
+"POST /t
+" .
+"pp\x02\x00\x00\x00\x00\x00\x00\x03ABC"
+--- stream_conf_enable
+--- error_log
+call pingpong's log
Review Comment:
This error log only means `protocol.log` is called, but not the logger.
--
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 #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #6960:
URL: https://github.com/apache/apisix/pull/6960
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r867596226
##########
t/xrpc/pingpong2.t:
##########
@@ -139,3 +151,548 @@ lua tcp socket send timeout: 60000
stream lua tcp socket read timeout: 60000
--- log_level: debug
--- stream_conf_enable
+
+
+
+=== TEST 3: bad loggger filter
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ {
+ protocol = {
+ name = "pingpong",
+ logger = {
+ {
+ name = "syslog",
+ filter = {
+ {}
+ },
+ conf = {}
+ }
+ }
+ },
+ upstream = {
+ nodes = {
+ ["127.0.0.1:1995"] = 1
+ },
+ type = "roundrobin"
+ }
+ }
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+
+
+
+=== TEST 4: failed to validate the 'filter' expression
+--- request eval
+"POST /t
+" .
+"pp\x02\x00\x00\x00\x00\x00\x00\x03ABC"
+--- stream_conf_enable
+--- error_log
+failed to validate the 'filter' expression: rule too short
+
+
+
+=== TEST 5: set loggger filter(single rule)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/stream_routes/1',
+ ngx.HTTP_PUT,
+ {
+ protocol = {
+ name = "pingpong",
+ logger = {
+ {
+ name = "syslog",
+ filter = {
+ {"len", ">", 10}
+ },
+ conf = {}
+ }
+ }
+ },
+ upstream = {
+ nodes = {
+ ["127.0.0.1:1995"] = 1
+ },
+ type = "roundrobin"
+ }
+ }
+ )
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+
+
+
+=== TEST 6: log filter matched successful
+--- request eval
+"POST /t
+" .
+"pp\x02\x00\x00\x00\x00\x00\x00\x03ABC"
+--- stream_conf_enable
+--- error_log
+call pingpong's log
Review Comment:
update
##########
apisix/schema_def.lua:
##########
@@ -807,6 +807,28 @@ local xrpc_protocol_schema = {
description = "protocol-specific configuration",
type = "object",
},
+ logger = {
+ type = "array",
+ items = {
+ properties = {
+ name = {
+ type = "string",
+ },
+ filter = {
+ description = "log filter rules",
Review Comment:
update
--
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] tzssangglass commented on a diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r867745588
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -93,11 +93,13 @@ local function run_log_plugin(ctx, logger)
local pkg_name = "apisix.stream.plugins." .. logger.name
local ok, plugin = pcall(require, pkg_name)
if not ok then
- core.log.error("failed to load plugin [", plugin, "] err: ", plugin)
+ core.log.error("failed to load plugin [", logger.name, "] err: ", plugin)
return
end
- core.ctx.set_vars_meta(ctx)
+ -- we choose to initialize conf_id and conf_type here
+ -- because conf here refers specifically to the conf in the log phase,
+ -- to avoid overwriting the conf in other parts of the protocol
ctx.conf_id = tostring(logger.conf)
ctx.conf_type = "xrpc-logger"
Review Comment:
update
##########
t/xrpc/apisix/stream/xrpc/protocols/pingpong/init.lua:
##########
@@ -76,6 +76,12 @@ local function to_int32(p, idx)
end
+local function init_ctx_fileds(ctx)
+ -- init some ctx fields
+ core.ctx.set_vars_meta(ctx)
Review Comment:
update
--
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 diff in pull request #6960: feat(xRPC): support log filter
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6960:
URL: https://github.com/apache/apisix/pull/6960#discussion_r869814674
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -71,9 +79,58 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger then
+ return false
+ end
+
+ if not logger.filter or #logger.filter == 0 then
+ -- no valid filter, default execution plugin
+ return true
+ end
+
+ -- key and version are divided by per-logger level
+ local key = "xrpc-logger" .. ctx.conf_id
Review Comment:
We can use ctx.conf_id directly?
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -14,14 +14,22 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
+local require = require
local core = require("apisix.core")
+local expr = require("resty.expr.v1")
local pairs = pairs
local ngx = ngx
local ngx_now = ngx.now
local OK = ngx.OK
local DECLINED = ngx.DECLINED
local DONE = ngx.DONE
+local pcall = pcall
+local ipairs = ipairs
+local tostring = tostring
+local logger_expr_cache = core.lrucache.new({
+ ttl = 300, count = 32
Review Comment:
Personally speaking, what about using a larger count like 1024?
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -71,9 +79,58 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger then
+ return false
+ end
+
+ if not logger.filter or #logger.filter == 0 then
+ -- no valid filter, default execution plugin
+ return true
+ end
+
+ -- key and version are divided by per-logger level
+ local key = "xrpc-logger" .. ctx.conf_id
+ local version = tostring(logger.filter)
+ local filter_expr, err = logger_expr_cache(key, version, expr.new, logger.filter)
+ if not filter_expr or err then
Review Comment:
```suggestion
if not filter_expr then
```
is enough?
##########
apisix/stream/xrpc/runner.lua:
##########
@@ -71,9 +79,58 @@ local function put_req_ctx(session, ctx)
end
+local function filter_logger(ctx, logger)
+ if not logger then
+ return false
+ end
+
+ if not logger.filter or #logger.filter == 0 then
+ -- no valid filter, default execution plugin
+ return true
+ end
+
+ -- key and version are divided by per-logger level
+ local key = "xrpc-logger" .. ctx.conf_id
+ local version = tostring(logger.filter)
Review Comment:
Actually, we can use the key directly as the version so we can save a `tostring`.
As each logger's conf's memory address is independent.
--
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