-
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: Added log format support in syslog plugin. #8279
feat: Added log format support in syslog plugin. #8279
Conversation
t/plugin/syslog.t
Outdated
@@ -132,8 +132,36 @@ passed | |||
[error] | |||
|
|||
|
|||
=== TEST 5: add plugin metadata |
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.
Could you check the logged data format?
You can refer to https://github.com/apache/apisix/pull/7328/files#diff-1108dce1e7823ca0b746c4ba69e692e4b02c6d49117d70859954b56cbb810c29 as an example.
BTW, new test should be added at the end.
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.
I fixed test reordering problem.
Should I add "access request" test?
And is that enough if is require?
=== TEST 10: access for log format
--- request
GET /hello
--- response_body
hello world
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.
Let's do it. And we need to inject some code in extra_init_by_lua
to check the logged data.
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.
I think we don't need extra_init_by_lua
for these tests.
I added new test cases. Please review it again.
BTW: It was to hard to understand test framework for me. :)
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.
BTW: It was to hard to understand test framework for me. :)
You can read the doc in https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md
dd52597
to
642c15c
Compare
t/plugin/syslog.t
Outdated
--- grep_error_log eval | ||
qr/sending a batch logs to 127.0.0.1:(\d+)/ | ||
--- grep_error_log_out | ||
sending a batch logs to 127.0.0.1:5044 |
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.
This check only ensures the log is sent to port 5044, which is already checked by other tests.
It is necessary to check the content of the log via injected code like
Line 238 in dc4b354
assert(r.client_ip == "127.0.0.1", r.client_ip) |
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.
I changed syslog server port.
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.
Simply modifying the port is useless , we should check the format of the reported logs to ensure that log_format is in effect, see: https://github.com/apache/apisix/blob/master/t/plugin/kafka-logger-log-format.t
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.
Could you please check again?
t/plugin/syslog.t
Outdated
--- no_error_log | ||
[error] | ||
--- error_log eval | ||
qr/syslog-log-format.*\{.*"host":"localhost"/ |
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.
I run this test locally and it can pass even without setting metadata, because the default log format already contains "host":"localhost"
in
apisix/apisix/utils/log-util.lua
Line 128 in ab4fe88
headers = ngx.req.get_headers(), |
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.
I added upstream variable into log format.
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
Please make the CI pass, thanks! |
* upstream/master: (48 commits) fix(ai): remove BUILD_ROUTER event when ai module is unloaded (apache#8184) chore: add some comment for make_request_to_vault function (apache#8420) docs: update admin api English doc (apache#8227) ci: use fixed os version of ubuntu (apache#8438) feat: Support store secrets in secrets manager for auth plugin via kms components (apache#8421) feat: interact via gRPC in APISIX Admin API (apache#8411) fix: last_err can be nil when the reconnection is successful (apache#8377) feat: support global data encryption of secret information (apache#8403) refactor(env): rename funtion name (apache#8426) feat(admin): add kms admin api (apache#8394) docs: update consumer and upstream docs (apache#8223) ci: add cron job for GM (apache#8398) docs: add kms env doc (apache#8419) feat: Added log format support in syslog plugin. (apache#8279) feat: add vault common components (apache#8412) docs: update global-rule/plugin-config/plugin/ docs (apache#8262) docs: update consumer-group/router/service/script doc (apache#8332) feat: support store secret in env for auth plugin (apache#8390) docs: update Upgrade Guide CN version (apache#8392) docs: add GM plugin EN doc to make website display normally (apache#8393) ...
Fixes #8278