feat(kafka-logger): supports logging request body by windyrjc · Pull Request #5501 · apache/apisix

windyrjc · 2021-11-14T03:47:41Z

What this PR does / why we need it:

kafka logger supports logging request body
fix #5343

Pre-submission checklist:

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 first

spacewander · 2021-11-14T12:02:24Z

        inactive_timeout = {type = "integer", minimum = 1, default = 5},
        batch_max_size = {type = "integer", minimum = 1, default = 1000},
        include_req_body = {type = "boolean", default = false},
+        request_body_expr = {


Would be better to name it as include_req_body_expr?

spacewander · 2021-11-14T12:17:41Z

+--- error_log_like eval
+qr/send data to kafka: \{.*"body":"abcdef"/
+--- wait: 2
+--- wait: 2


Duplicate --- wait?

spacewander · 2021-11-14T12:18:23Z

+        local log_request_body = true
+
+        if conf.request_body_expr then
+            local request_expr, err = expr.new(conf.request_body_expr)


We can check conf.request_expr before new?

spacewander · 2021-11-14T12:20:05Z

+        if conf.request_body_expr then
+            local request_expr, err = expr.new(conf.request_body_expr)
+            if not request_expr then
+                core.log.error('generate log expr err ' .. err)


Need to skip the remaining logic. We should not use a nil request_expr.

spacewander · 2021-11-15T01:39:03Z

+                                "include_req_body": true,
+                                "request_body_expr": [
+                                    [
+                                      "remote_addr",


Better to use a test case that can eval to false.

spacewander · 2021-11-15T01:39:31Z

+
+
+
+=== TEST 27: hit route, report log to kafka


Let's add a test that expr is evaluated to false

spacewander · 2021-11-15T01:42:22Z

 | 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. |
 | cluster_name     | integer | optional    | 1              | [0,...] | the name of the cluster. When there are two or more kafka clusters, you can specify different names. And this only works with async producer_type.|
+| request_body_expr  | object  | optional    |                |         | Whether to logging request body,based on [lua-resty-expr](https://github.com/api7/lua-resty-expr).            |


Better to move it under include_req_body and explain its relation to include_req_body.

spacewander · 2021-11-15T03:13:45Z

-                                "request_body_expr": [
+                                "include_req_body_expr": [
                                    [
                                      "remote_addr",


We can do it with arg_xx. There is no need to use a separate rule.

I'm not understand what this means, please be more specific?, thank you. 😁

Use arg_blah == 1 in the expression, and hit it with "POST /hello?blah=1" and "POST /hello?blah=2".

I'm using ctx.var to eval the expr and ctx.var doesn't directly hold args. it only has args table. If you want to achieve this, i need to eval the expr by ctx.var.args

I mean to use $arg_ variable. You can refer to

apisix/t/plugin/traffic-split4.t

Line 110 in cc43b9f

"vars": [["arg_name", "==", "James"]]

I mean to use $arg_ variable. You can refer to

apisix/t/plugin/traffic-split4.t

Line 110 in cc43b9f

"vars": [["arg_name", "==", "James"]]

done ,thank you very much

spacewander · 2021-11-15T03:25:57Z

+
+=== TEST 28: hit route, not trigger request_body_expr rule
+--- request
+GET /hello


The test will pass whether the request_body_expr is correct or not

Ok, i just want to prove that if there was no request body, it still work normally. There is already has a eval false case below and i'm delete this case for good

spacewander · 2021-11-15T03:28:23Z

+                        },
+                        "uri": "/hello"
+                }]],
+                [[{


Please remove the part which is not relative to the test, from L1156 to L1182

tzssangglass · 2021-11-16T06:18:21Z

 | max_retry_count  | integer | 可选   | 0              | [0,...] | 从处理管道中移除之前的最大重试次数。             |
 | retry_delay      | integer | 可选   | 1              | [0,...] | 如果执行失败，则应延迟执行流程的秒数。           |
 | include_req_body | boolean | 可选   | false          | [false, true] | 是否包括请求 body。false： 表示不包含请求的 body ； true： 表示包含请求的 body 。|
+| include_req_body_expr | array  | 可选    |           |         | 是否采集请求body,基于[lua-resty-expr](https://github.com/api7/lua-resty-expr)。 该选项需要开启 `include_req_body`|


Suggested change

| include_req_body_expr | array | 可选 | | | 是否采集请求body,基于[lua-resty-expr](https://github.com/api7/lua-resty-expr)。该选项需要开启 `include_req_body`|

| include_req_body_expr | array | 可选 | | | 是否采集请求body，基于 [lua-resty-expr](https://github.com/api7/lua-resty-expr)。该选项需要开启 `include_req_body`|

tzssangglass · 2021-11-16T06:18:57Z

 | 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_req_body_expr  | array  | optional    |          |         | Whether to logging request body,based on [lua-resty-expr](https://github.com/api7/lua-resty-expr), this option require to turn on `include_req_body` option.             |


Suggested change

| include_req_body_expr | array | optional | | | Whether to logging request body,based on [lua-resty-expr](https://github.com/api7/lua-resty-expr), this option require to turn on `include_req_body` option. |

| include_req_body_expr | array | optional | | | Whether to logging request body, based on [lua-resty-expr](https://github.com/api7/lua-resty-expr), this option require to turn on `include_req_body` option. |

tzssangglass · 2021-11-16T06:27:28Z

+--- wait: 2
+
+
+=== TEST 28: hit route,expr eval fail


Suggested change

--- wait: 2

=== TEST 28: hit route,expr eval fail

--- wait: 2

=== TEST 28: hit route,expr eval fail

tzssangglass · 2021-11-16T06:28:03Z

+[error]
+
+
+=== TEST 27: hit route, expr eval success


Suggested change

[error]

=== TEST 27: hit route, expr eval success

[error]

=== TEST 27: hit route, expr eval success

i forgot lint before push, sorry~

tzssangglass · 2021-11-16T07:28:02Z

+        include_req_body_expr = {
+            type = "array",
+            items = {
+                type = "array",
+                items = {
+                    type = "string"
+                }
+            }
+        },


Suggested change

include_req_body_expr = {

type = "array",

items = {

type = "array",

items = {

type = "string"

}

}

},

include_req_body_expr = {

type = "array",

minItems = 1,

items = {

type = "array",

items = {

type = "string"

}

}

},

tzssangglass · 2021-11-16T07:28:25Z

+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                 ngx.HTTP_PUT,
+                 [[{


Suggested change

[[{

[=[{

tzssangglass · 2021-11-16T07:28:37Z

+                            "type": "roundrobin"
+                        },
+                        "uri": "/hello"
+                }]]


Suggested change

}]]

}]=]

spacewander · 2021-11-16T09:13:38Z

        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_req_body_expr = {


Before we can merge it, could you add a check for the expr in the check_schema? Like:

apisix/apisix/plugins/response-rewrite.lua

Line 111 in 9b355c6

if conf.vars then

spacewander · 2021-11-17T01:02:11Z


 function _M.check_schema(conf, schema_type)
+
+    if conf.vars then


Should be include_req_body_expr

feat(kafka-logger) kafka logger supports logging request body (apache…

613c19e

…#5343)

windyrjc changed the title ~~feat(kafka-logger) kafka logger supports logging request body (#5343)~~ feat: kafka logger supports logging request body (#5343) Nov 14, 2021

windyrjc mentioned this pull request Nov 14, 2021

request help: kafka logger supports logging request body #5343

Closed

reindex

acb54e3

spacewander reviewed Nov 14, 2021

View reviewed changes

windyrjc added 2 commits November 14, 2021 21:02

optimize

fb04109

change test kafka topic test2

d922ffd

spacewander reviewed Nov 15, 2021

View reviewed changes

optimize

3e867ea

spacewander reviewed Nov 15, 2021

View reviewed changes

optimize test

f691471

windyrjc requested a review from spacewander November 16, 2021 02:58

tzssangglass mentioned this pull request Nov 16, 2021

request help: kafka logger supports logging response body #5344

Closed

tzssangglass reviewed Nov 16, 2021

View reviewed changes

lint

d82854c

windyrjc requested a review from tzssangglass November 16, 2021 06:39

tzssangglass reviewed Nov 16, 2021

View reviewed changes

spacewander approved these changes Nov 16, 2021

View reviewed changes

optimize

a017631

windyrjc requested a review from tzssangglass November 16, 2021 10:20

tzssangglass approved these changes Nov 16, 2021

View reviewed changes

spacewander reviewed Nov 17, 2021

View reviewed changes

optimize

7ea1b17

windyrjc requested a review from spacewander November 17, 2021 11:11

lint

d31a5ab

spacewander approved these changes Nov 18, 2021

View reviewed changes

spacewander changed the title ~~feat: kafka logger supports logging request body (#5343)~~ feat(kafka-logger): supports logging request body Nov 18, 2021

spacewander merged commit 5394dce into apache:master Nov 18, 2021

windyrjc deleted the #5343 branch November 18, 2021 02:01

	\| include_req_body_expr \| array \| 可选 \| \| \| 是否采集请求body,基于[lua-resty-expr](https://github.com/api7/lua-resty-expr)。该选项需要开启 `include_req_body`\|
	\| include_req_body_expr \| array \| 可选 \| \| \| 是否采集请求body，基于 [lua-resty-expr](https://github.com/api7/lua-resty-expr)。该选项需要开启 `include_req_body`\|

	\| include_req_body_expr \| array \| optional \| \| \| Whether to logging request body,based on [lua-resty-expr](https://github.com/api7/lua-resty-expr), this option require to turn on `include_req_body` option. \|
	\| include_req_body_expr \| array \| optional \| \| \| Whether to logging request body, based on [lua-resty-expr](https://github.com/api7/lua-resty-expr), this option require to turn on `include_req_body` option. \|


		function _M.check_schema(conf, schema_type)

		if conf.vars then

Uh oh!

Conversation

windyrjc commented Nov 14, 2021 • edited by spacewander Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windyrjc Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windyrjc Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windyrjc commented Nov 14, 2021 •

edited by spacewander

Loading

windyrjc Nov 15, 2021 •

edited

Loading

windyrjc Nov 15, 2021 •

edited

Loading