feat(http/kafka-logger): support to log response body #5550
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spacewander
reviewed
Nov 19, 2021
apisix/plugins/http-logger.lua
Outdated
| @@ -162,6 +163,17 @@ local function remove_stale_objects(premature) | |||
| end | |||
|
|
|||
|
|
|||
| function _M.body_filter(conf, ctx) | |||
| if conf.include_resp_body then | |||
There was a problem hiding this comment.
Let's refactor this part into a method in log-util.
tokers
approved these changes
Nov 19, 2021
|
Hi @dmsolr, please update your PR to resolve those conflicts 😄 Thanks! |
spacewander
reviewed
Nov 23, 2021
apisix/plugins/http-logger.lua
Outdated
| @@ -70,6 +80,11 @@ local _M = { | |||
|
|
|||
|
|
|||
| function _M.check_schema(conf, schema_type) | |||
| local ok, err = log_util.check_log_scheme(conf) | |||
There was a problem hiding this comment.
We should not check log schema with the TYPE_METADATA schema_type. And it is schema, not scheme.
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
spacewander
reviewed
Nov 23, 2021
| @@ -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 的限制，我们没有办法把它记录下来。| | |||
There was a problem hiding this comment.
The old change is overrided.
| @@ -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`. | | |||
There was a problem hiding this comment.
Missing doc of include_resp_body_expr
apisix/utils/log-util.lua
Outdated
| conf.response_expr = response_expr | ||
| end | ||
|
|
||
| local result = conf.response_expr:eval(ctx.var) |
There was a problem hiding this comment.
Need to cache eval result to the ctx
apisix/plugins/kafka-logger.lua
Outdated
| 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) |
There was a problem hiding this comment.
We should run core.schema.check first before check_log_schema
apisix/plugins/kafka-logger.lua
Outdated
|
|
||
| local ok, err = log_util.check_log_schema(conf) | ||
| if not ok then | ||
| return err |
There was a problem hiding this comment.
Suggested change
| return err | |
| return nil, err |
apisix/utils/log-util.lua
Outdated
| 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} |
There was a problem hiding this comment.
We should return err.
And please add a test to cover this.
spacewander
reviewed
Nov 25, 2021
| @@ -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 。| | |||
There was a problem hiding this comment.
注意：如果请求 body 没办法完全放在内存中，由于 Nginx 的限制，我们没有办法把它记录下来。 This part is overridden.
There was a problem hiding this comment.
Sorry.
I think there is the same limitation in the http-logger plugin. Right?
t/plugin/http-logger.t
Outdated
| @@ -108,7 +108,43 @@ done | |||
|
|
|||
|
|
|||
|
|
|||
| === TEST 4: add plugin | |||
| === TEST 4: check log schema(include_resp_body_expr) | |||
There was a problem hiding this 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.
spacewander
approved these changes
Nov 26, 2021
spacewander
changed the title
besich
pushed a commit
to besich/apisix
that referenced
this pull request
Dec 1, 2021
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Pre-submission checklist:
Closes #5344