-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Changes from 4 commits
613c19e
acb54e3
fb04109
d922ffd
3e867ea
f691471
d82854c
a017631
7ea1b17
d31a5ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line 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 = { | ||||||||||||||||||||||||||||||||||||||||
type = "array", | ||||||||||||||||||||||||||||||||||||||||
items = { | ||||||||||||||||||||||||||||||||||||||||
type = "array", | ||||||||||||||||||||||||||||||||||||||||
items = { | ||||||||||||||||||||||||||||||||||||||||
type = "string" | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
-- in lua-resty-kafka, cluster_name is defined as number | ||||||||||||||||||||||||||||||||||||||||
-- see https://github.com/doujiang24/lua-resty-kafka#new-1 | ||||||||||||||||||||||||||||||||||||||||
cluster_name = {type = "integer", minimum = 1, default = 1}, | ||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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). | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
### examples of meta_format | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1114,3 +1114,97 @@ GET /t | |||||
--- error_log_like eval | ||||||
qr/create new kafka producer instance, brokers: \[\{"port":9092,"host":"127.0.0.127"}]/ | ||||||
qr/failed to send data to Kafka topic: .*, brokers: \{"127.0.0.127":9092}/ | ||||||
|
||||||
|
||||||
|
||||||
=== TEST 26: set route(id: 1,include_req_body = true,request_body_expr = array) | ||||||
--- config | ||||||
location /t { | ||||||
content_by_lua_block { | ||||||
local t = require("lib.test_admin").test | ||||||
local code, body = t('/apisix/admin/routes/1', | ||||||
ngx.HTTP_PUT, | ||||||
[[{ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
"plugins": { | ||||||
"kafka-logger": { | ||||||
"broker_list" : | ||||||
{ | ||||||
"127.0.0.1":9092 | ||||||
}, | ||||||
"kafka_topic" : "test2", | ||||||
"key" : "key1", | ||||||
"timeout" : 1, | ||||||
"include_req_body": true, | ||||||
"request_body_expr": [ | ||||||
[ | ||||||
"remote_addr", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better to use a test case that can eval to false. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do it with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean to use apisix/t/plugin/traffic-split4.t Line 110 in cc43b9f
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
done ,thank you very much |
||||||
"==", | ||||||
"127.0.0.1" | ||||||
] | ||||||
], | ||||||
"batch_max_size": 1 | ||||||
} | ||||||
}, | ||||||
"upstream": { | ||||||
"nodes": { | ||||||
"127.0.0.1:1980": 1 | ||||||
}, | ||||||
"type": "roundrobin" | ||||||
}, | ||||||
"uri": "/hello" | ||||||
}]], | ||||||
[[{ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
"node": { | ||||||
"value": { | ||||||
"plugins": { | ||||||
"kafka-logger": { | ||||||
"broker_list" : | ||||||
{ | ||||||
"127.0.0.1":9092 | ||||||
}, | ||||||
"kafka_topic" : "test2", | ||||||
"key" : "key1", | ||||||
"timeout" : 1, | ||||||
"batch_max_size": 1 | ||||||
} | ||||||
}, | ||||||
"upstream": { | ||||||
"nodes": { | ||||||
"127.0.0.1:1980": 1 | ||||||
}, | ||||||
"type": "roundrobin" | ||||||
}, | ||||||
"uri": "/hello" | ||||||
}, | ||||||
"key": "/apisix/routes/1" | ||||||
}, | ||||||
"action": "set" | ||||||
}]] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
) | ||||||
if code >= 300 then | ||||||
ngx.status = code | ||||||
end | ||||||
ngx.say(body) | ||||||
} | ||||||
} | ||||||
|
||||||
--- request | ||||||
GET /t | ||||||
--- response_body | ||||||
passed | ||||||
--- no_error_log | ||||||
[error] | ||||||
|
||||||
|
||||||
|
||||||
=== TEST 27: hit route, report log to kafka | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add a test that expr is evaluated to false There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
--- request | ||||||
POST /hello | ||||||
abcdef | ||||||
--- response_body | ||||||
hello world | ||||||
--- no_error_log | ||||||
[error] | ||||||
--- error_log_like eval | ||||||
qr/send data to kafka: \{.*"body":"abcdef"/ | ||||||
--- wait: 2 |
There was a problem hiding this comment.
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:apisix/apisix/plugins/response-rewrite.lua
Line 111 in 9b355c6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done