Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(kafka-logger): supports logging request body #5501

Merged
merged 10 commits into from
Nov 18, 2021

Conversation

windyrjc
Copy link
Contributor

@windyrjc windyrjc commented Nov 14, 2021

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

@windyrjc 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
@@ -74,6 +74,15 @@ 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},
request_body_expr = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to name it as include_req_body_expr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

apisix/utils/log-util.lua Show resolved Hide resolved
--- error_log_like eval
qr/send data to kafka: \{.*"body":"abcdef"/
--- wait: 2
--- wait: 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate --- wait?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

local log_request_body = true

if conf.request_body_expr then
local request_expr, err = expr.new(conf.request_body_expr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can check conf.request_expr before new?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"include_req_body": true,
"request_body_expr": [
[
"remote_addr",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done




=== TEST 27: hit route, report log to kafka
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -58,6 +58,7 @@ For more info on Batch-Processor in Apache APISIX please refer.
| 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). |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -1135,7 +1135,7 @@ qr/failed to send data to Kafka topic: .*, brokers: \{"127.0.0.127":9092}/
"key" : "key1",
"timeout" : 1,
"include_req_body": true,
"request_body_expr": [
"include_req_body_expr": [
[
"remote_addr",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@windyrjc windyrjc Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@windyrjc windyrjc Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

done ,thank you very much


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test will pass whether the request_body_expr is correct or not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
"uri": "/hello"
}]],
[[{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -57,6 +57,7 @@ title: kafka-logger
| 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`|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`|

@@ -57,6 +57,7 @@ For more info on Batch-Processor in Apache APISIX please refer.
| 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. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. |

Comment on lines 1182 to 1185
--- wait: 2


=== TEST 28: hit route,expr eval fail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 1169 to 1172
[error]


=== TEST 27: hit route, expr eval success
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i forgot lint before push, sorry~

Comment on lines 77 to 85
include_req_body_expr = {
type = "array",
items = {
type = "array",
items = {
type = "string"
}
}
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
}
}
},

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[[{
[=[{

"type": "roundrobin"
},
"uri": "/hello"
}]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}]]
}]=]

@@ -74,6 +74,15 @@ 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_req_body_expr = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if conf.vars then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -107,6 +109,14 @@ local _M = {


function _M.check_schema(conf, schema_type)

if conf.vars then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be include_req_body_expr

@spacewander spacewander changed the title feat: kafka logger supports logging request body (#5343) feat(kafka-logger): supports logging request body Nov 18, 2021
@spacewander spacewander merged commit 5394dce into apache:master Nov 18, 2021
@windyrjc windyrjc deleted the #5343 branch November 18, 2021 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request help: kafka logger supports logging request body
3 participants