feat(http/kafka-logger): support to log response body#5550
Merged
Conversation
spacewander
reviewed
Nov 19, 2021
|
|
||
|
|
||
| function _M.body_filter(conf, ctx) | ||
| if conf.include_resp_body then |
Member
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
Member
|
Hi @dmsolr, please update your PR to resolve those conflicts 😄 Thanks! |
spacewander
reviewed
Nov 23, 2021
|
|
||
|
|
||
| function _M.check_schema(conf, schema_type) | ||
| local ok, err = log_util.check_log_scheme(conf) |
Member
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: 罗泽轩 <[email protected]>
spacewander
reviewed
Nov 23, 2021
| | 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 的限制，我们没有办法把它记录下来。| |
Member
There was a problem hiding this comment.
The old change is overrided.
| | 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`. | |
Member
There was a problem hiding this comment.
Missing doc of include_resp_body_expr
| conf.response_expr = response_expr | ||
| end | ||
|
|
||
| local result = conf.response_expr:eval(ctx.var) |
Member
There was a problem hiding this comment.
Need to cache eval result to the ctx
| return core.schema.check(metadata_schema, conf) | ||
| end | ||
|
|
||
| local ok, err = log_util.check_log_schema(conf) |
Member
There was a problem hiding this comment.
We should run core.schema.check first before check_log_schema
|
|
||
| local ok, err = log_util.check_log_schema(conf) | ||
| if not ok then | ||
| return err |
Member
There was a problem hiding this comment.
Suggested change
| return err | |
| return nil, err |
| 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} |
Member
There was a problem hiding this comment.
We should return err.
And please add a test to cover this.
added 2 commits
November 24, 2021 21:17
spacewander
reviewed
Nov 25, 2021
| | 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 。| |
Member
There was a problem hiding this comment.
注意：如果请求 body 没办法完全放在内存中，由于 Nginx 的限制，我们没有办法把它记录下来。 This part is overridden.
Member
Author
There was a problem hiding this comment.
Sorry.
I think there is the same limitation in the http-logger plugin. Right?
|
|
||
|
|
||
| === TEST 4: add plugin | ||
| === TEST 4: check log schema(include_resp_body_expr) |
Member
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
bd1dtn
pushed a commit
to bd1dtn/apisix
that referenced
this pull request
Dec 1, 2021
Co-authored-by: 罗泽轩 <[email protected]>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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