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 2021/11/19 01:22:51 UTC
[GitHub] [apisix] dmsolr opened a new pull request #5550: feat: support to log response body
dmsolr opened a new pull request #5550:
URL: https://github.com/apache/apisix/pull/5550
### 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 requirements:
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. Use "request review" to notify the reviewer once you have resolved the review
-->
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] 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**
Closes #5344
--
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 #5550: feat(http/kafka-logger): support to log response body
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #5550:
URL: https://github.com/apache/apisix/pull/5550
--
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] dmsolr commented on a change in pull request #5550: feat: support to log response body
Posted by GitBox <gi...@apache.org>.
dmsolr commented on a change in pull request #5550:
URL: https://github.com/apache/apisix/pull/5550#discussion_r756529615
##########
File path: docs/zh/latest/plugins/kafka-logger.md
##########
@@ -56,8 +56,10 @@ title: kafka-logger
| buffer_duration | integer | 可选 | 60 | [1,...] | 必须先处理批次中最旧条目的最长期限（以秒为单位）。 |
| max_retry_count | integer | 可选 | 0 | [0,...] | 从处理管道中移除之前的最大重试次数。 |
| retry_delay | integer | 可选 | 1 | [0,...] | 如果执行失败，则应延迟执行流程的秒数。 |
-| include_req_body | boolean | 可选 | false | [false, true] | 是否包括请求 body。false： 表示不包含请求的 body ；true： 表示包含请求的 body。注意：如果请求 body 没办法完全放在内存中，由于 Nginx 的限制，我们没有办法把它记录下来。|
-| include_req_body_expr | array | 可选 | | | 当 `include_req_body` 开启时, 基于 [lua-resty-expr](https://github.com/api7/lua-resty-expr) 表达式的结果进行记录。如果该选项存在，只有在表达式为真的时候才会记录请求 body。 |
+| include_req_body | boolean | 可选 | false | [false, true] | 是否包括请求 body。false： 表示不包含请求的 body ； true： 表示包含请求的 body 。|
Review comment:
Sorry.
I think there is the same limitation in the http-logger plugin. Right?
--
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 #5550: feat: support to log response body
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5550:
URL: https://github.com/apache/apisix/pull/5550#discussion_r756517533
##########
File path: docs/zh/latest/plugins/kafka-logger.md
##########
@@ -56,8 +56,10 @@ title: kafka-logger
| buffer_duration | integer | 可选 | 60 | [1,...] | 必须先处理批次中最旧条目的最长期限（以秒为单位）。 |
| max_retry_count | integer | 可选 | 0 | [0,...] | 从处理管道中移除之前的最大重试次数。 |
| retry_delay | integer | 可选 | 1 | [0,...] | 如果执行失败，则应延迟执行流程的秒数。 |
-| include_req_body | boolean | 可选 | false | [false, true] | 是否包括请求 body。false： 表示不包含请求的 body ；true： 表示包含请求的 body。注意：如果请求 body 没办法完全放在内存中，由于 Nginx 的限制，我们没有办法把它记录下来。|
-| include_req_body_expr | array | 可选 | | | 当 `include_req_body` 开启时, 基于 [lua-resty-expr](https://github.com/api7/lua-resty-expr) 表达式的结果进行记录。如果该选项存在，只有在表达式为真的时候才会记录请求 body。 |
+| include_req_body | boolean | 可选 | false | [false, true] | 是否包括请求 body。false： 表示不包含请求的 body ； true： 表示包含请求的 body 。|
Review comment:
`注意：如果请求 body 没办法完全放在内存中，由于 Nginx 的限制，我们没有办法把它记录下来。` This part is overridden.
##########
File path: t/plugin/http-logger.t
##########
@@ -108,7 +108,43 @@ done
-=== TEST 4: add plugin
+=== TEST 4: check log schema(include_resp_body_expr)
Review comment:
Could we add the test at the end, so the other test numbers won't be changed? We can use `reindex` to fix the number.
--
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 #5550: feat: support to log response body
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5550:
URL: https://github.com/apache/apisix/pull/5550#discussion_r754846203
##########
File path: apisix/utils/log-util.lua
##########
@@ -196,4 +200,55 @@ function _M.latency_details_in_ms(ctx)
return latency, upstream_latency, apisix_latency
end
+
+function _M.check_log_schema(conf)
+ if conf.include_req_body_expr then
+ local ok, err = expr.new(conf.include_req_body_expr)
+ if not ok then
+ return nil,
+ {error_msg = "failed to validate the 'include_req_body_expr' expression: " .. err}
+ end
+ end
+ if conf.include_resp_body_expr then
+ local ok, err = expr.new(conf.include_resp_body_expr)
+ if not ok then
+ return nil,
+ {error_msg = "failed to validate the 'include_resp_body_expr' expression: " .. err}
+ end
+ end
+ return true, nil
+end
+
+
+function _M.collect_body(conf, ctx)
+ if conf.include_resp_body then
+ local log_response_body = true
+
+ if conf.include_resp_body_expr then
+ if not conf.response_expr then
+ local response_expr, err = expr.new(conf.include_resp_body_expr)
+ if not response_expr then
+ core.log.error('generate response expr err ' .. err)
+ return
+ end
+ conf.response_expr = response_expr
+ end
+
+ local result = conf.response_expr:eval(ctx.var)
Review comment:
Need to cache eval result to the ctx
##########
File path: docs/en/latest/plugins/http-logger.md
##########
@@ -50,6 +50,7 @@ This will provide the ability to send Log data requests as JSON objects to Monit
| max_retry_count | integer | optional | 0 | [0,...] | Maximum number of retries before removing from the processing pipe line. |
| retry_delay | integer | optional | 1 | [0,...] | Number of seconds the process execution should be delayed if the execution fails. |
| include_req_body | boolean | optional | false | [false, true] | Whether to include the request body. false: indicates that the requested body is not included; true: indicates that the requested body is included. |
+| include_resp_body| boolean | optional | false | [false, true] | Whether to include the response body. The response body is included if and only if it is `true`. |
Review comment:
Missing doc of include_resp_body_expr
##########
File path: apisix/plugins/kafka-logger.lua
##########
@@ -109,18 +119,14 @@ local _M = {
function _M.check_schema(conf, schema_type)
-
- if conf.include_req_body_expr then
- local ok, err = expr.new(conf.include_req_body_expr)
- if not ok then
- return nil,
- {error_msg = "failed to validate the 'include_req_body_expr' expression: " .. err}
- end
- end
-
if schema_type == core.schema.TYPE_METADATA then
return core.schema.check(metadata_schema, conf)
end
+
+ local ok, err = log_util.check_log_schema(conf)
+ if not ok then
+ return err
Review comment:
```suggestion
return nil, err
```
##########
File path: docs/zh/latest/plugins/kafka-logger.md
##########
@@ -56,8 +56,9 @@ title: kafka-logger
| buffer_duration | integer | 可选 | 60 | [1,...] | 必须先处理批次中最旧条目的最长期限（以秒为单位）。 |
| max_retry_count | integer | 可选 | 0 | [0,...] | 从处理管道中移除之前的最大重试次数。 |
| retry_delay | integer | 可选 | 1 | [0,...] | 如果执行失败，则应延迟执行流程的秒数。 |
-| include_req_body | boolean | 可选 | false | [false, true] | 是否包括请求 body。false： 表示不包含请求的 body ；true： 表示包含请求的 body。注意：如果请求 body 没办法完全放在内存中，由于 Nginx 的限制，我们没有办法把它记录下来。|
Review comment:
The old change is overrided.
##########
File path: apisix/utils/log-util.lua
##########
@@ -196,4 +200,55 @@ function _M.latency_details_in_ms(ctx)
return latency, upstream_latency, apisix_latency
end
+
+function _M.check_log_schema(conf)
+ if conf.include_req_body_expr then
+ local ok, err = expr.new(conf.include_req_body_expr)
+ if not ok then
+ return nil,
+ {error_msg = "failed to validate the 'include_req_body_expr' expression: " .. err}
Review comment:
We should return err.
And please add a test to cover this.
##########
File path: apisix/plugins/kafka-logger.lua
##########
@@ -109,18 +119,14 @@ local _M = {
function _M.check_schema(conf, schema_type)
-
- if conf.include_req_body_expr then
- local ok, err = expr.new(conf.include_req_body_expr)
- if not ok then
- return nil,
- {error_msg = "failed to validate the 'include_req_body_expr' expression: " .. err}
- end
- end
-
if schema_type == core.schema.TYPE_METADATA then
return core.schema.check(metadata_schema, conf)
end
+
+ local ok, err = log_util.check_log_schema(conf)
Review comment:
We should run `core.schema.check` first before `check_log_schema`
--
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] juzhiyuan commented on pull request #5550: feat: support to log response body
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #5550:
URL: https://github.com/apache/apisix/pull/5550#issuecomment-974651310
Hi @dmsolr, please update your PR to resolve those conflicts 😄 Thanks!
--
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] juzhiyuan commented on pull request #5550: feat: support to log response body
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #5550:
URL: https://github.com/apache/apisix/pull/5550#issuecomment-974651310
Hi @dmsolr, please update your PR to resolve those conflicts 😄 Thanks!
--
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] dmsolr commented on a change in pull request #5550: feat: support to log response body
Posted by GitBox <gi...@apache.org>.
dmsolr commented on a change in pull request #5550:
URL: https://github.com/apache/apisix/pull/5550#discussion_r756558576
##########
File path: t/plugin/http-logger.t
##########
@@ -108,7 +108,43 @@ done
-=== TEST 4: add plugin
+=== TEST 4: check log schema(include_resp_body_expr)
Review comment:
updated
--
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 #5550: feat: support to log response body
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5550:
URL: https://github.com/apache/apisix/pull/5550#discussion_r752796481
##########
File path: apisix/plugins/http-logger.lua
##########
@@ -44,6 +44,7 @@ local schema = {
inactive_timeout = {type = "integer", minimum = 1, default = 5},
batch_max_size = {type = "integer", minimum = 1, default = 1000},
include_req_body = {type = "boolean", default = false},
+ include_resp_body = {type = "boolean", default = false},
Review comment:
It would be better to use `include_resp_body_expr` to include the response body dynamically. We can eval it in the header_filter.
##########
File path: apisix/plugins/http-logger.lua
##########
@@ -162,6 +163,17 @@ local function remove_stale_objects(premature)
end
+function _M.body_filter(conf, ctx)
+ if conf.include_resp_body then
Review comment:
Let's refactor this part into a method in log-util.
--
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 #5550: feat: support to log response body
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5550:
URL: https://github.com/apache/apisix/pull/5550#discussion_r754744833
##########
File path: apisix/utils/log-util.lua
##########
@@ -203,4 +200,55 @@ function _M.latency_details_in_ms(ctx)
return latency, upstream_latency, apisix_latency
end
+
+function _M.check_log_scheme(conf)
+ if conf.include_req_body_expr then
+ local ok, err = expr.new(conf.include_req_body_expr)
+ if not ok then
+ return nil,
+ {error_msg = "failed to validate the 'include_req_body_expr' expression: " .. err}
+ end
+ end
+ if conf.include_resp_body_expr then
+ local ok, err = expr.new(conf.include_resp_body_expr)
+ if not ok then
+ return nil,
+ {error_msg = "failed to validate the 'include_resp_body_expr' expression: " .. err}
+ end
+ end
+ return true, nil
+end
+
+
+function _M.collect_body(conf, ctx)
+ if conf.include_resp_body then
+ local log_response_body = true
+
+ if not conf.include_resp_body_expr then
Review comment:
```suggestion
if conf.include_resp_body_expr then
```
We should keep the result to avoid reevaluating the expr.
##########
File path: apisix/utils/log-util.lua
##########
@@ -154,11 +154,8 @@ local function get_full_log(ngx, conf)
end
end
- if conf.include_resp_body then
- local body = ctx.resp_body
- if body then
- log.response.body = body
- end
+ if not ctx.resp_body then
Review comment:
```suggestion
if ctx.resp_body then
```
##########
File path: apisix/plugins/http-logger.lua
##########
@@ -70,6 +80,11 @@ local _M = {
function _M.check_schema(conf, schema_type)
+ local ok, err = log_util.check_log_scheme(conf)
Review comment:
We should not check log schema with the TYPE_METADATA schema_type. And it is schema, not scheme.
--
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 #5550: feat: support to log response body
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5550:
URL: https://github.com/apache/apisix/pull/5550#discussion_r757204230
##########
File path: docs/zh/latest/plugins/kafka-logger.md
##########
@@ -56,8 +56,10 @@ title: kafka-logger
| buffer_duration | integer | 可选 | 60 | [1,...] | 必须先处理批次中最旧条目的最长期限（以秒为单位）。 |
| max_retry_count | integer | 可选 | 0 | [0,...] | 从处理管道中移除之前的最大重试次数。 |
| retry_delay | integer | 可选 | 1 | [0,...] | 如果执行失败，则应延迟执行流程的秒数。 |
-| include_req_body | boolean | 可选 | false | [false, true] | 是否包括请求 body。false： 表示不包含请求的 body ；true： 表示包含请求的 body。注意：如果请求 body 没办法完全放在内存中，由于 Nginx 的限制，我们没有办法把它记录下来。|
-| include_req_body_expr | array | 可选 | | | 当 `include_req_body` 开启时, 基于 [lua-resty-expr](https://github.com/api7/lua-resty-expr) 表达式的结果进行记录。如果该选项存在，只有在表达式为真的时候才会记录请求 body。 |
+| include_req_body | boolean | 可选 | false | [false, true] | 是否包括请求 body。false： 表示不包含请求的 body ； true： 表示包含请求的 body 。|
Review comment:
Yes
--
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