-
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 1 commit
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 |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
-- limitations under the License. | ||
-- | ||
local core = require("apisix.core") | ||
local expr = require("resty.expr.v1") | ||
local ngx = ngx | ||
local pairs = pairs | ||
local str_byte = string.byte | ||
|
@@ -119,13 +120,32 @@ local function get_full_log(ngx, conf) | |
} | ||
|
||
if conf.include_req_body then | ||
spacewander marked this conversation as resolved.
Show resolved
Hide resolved
|
||
local body = req_get_body_data() | ||
if body then | ||
log.request.body = body | ||
else | ||
local body_file = ngx.req.get_body_file() | ||
if body_file then | ||
log.request.body_file = body_file | ||
|
||
local log_request_body = true | ||
|
||
if conf.request_body_expr then | ||
local request_expr, err = expr.new(conf.request_body_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. We can check 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 |
||
if not request_expr then | ||
core.log.error('generate log expr err ' .. err) | ||
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. Need to skip the remaining logic. We should not use a nil request_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. done |
||
end | ||
conf.request_expr = request_expr | ||
|
||
local result = conf.request_expr:eval(ctx.var) | ||
|
||
if not result then | ||
log_request_body = false | ||
end | ||
end | ||
|
||
if log_request_body then | ||
local body = req_get_body_data() | ||
if body then | ||
log.request.body = body | ||
else | ||
local body_file = ngx.req.get_body_file() | ||
if body_file then | ||
log.request.body_file = body_file | ||
end | ||
end | ||
end | ||
end | ||
|
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" : "test1", | ||||||
"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" : "test1", | ||||||
"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 | ||||||
--- wait: 2 | ||||||
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. Duplicate 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 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
?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