-
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: enable L4 stream logging #5768
Conversation
conf/config-default.yaml
Outdated
@@ -174,6 +174,11 @@ nginx_config: # config for render the template to generate n | |||
# - TEST_ENV | |||
|
|||
stream: | |||
enable_access_log: true # enable access log or not, default true | |||
access_log: logs/access_stream.log | |||
access_log_format: "$remote_addr [$time_local] $status $bytes_sent $session_time $upstream_connect_time \"$protocol://$upstream_addr\"" |
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.
@spacewander by consulting http://nginx.org/en/docs/varindex.html, I created the log format. Do you think we can do better? Do let me know. Thanks.
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 can use Nginx's basic format in its example?
log_format basic '$remote_addr [$time_local] '
'$protocol $status $bytes_sent $bytes_received '
'$session_time';
See https://nginx.org/en/docs/stream/ngx_stream_log_module.html.
Kong also uses it too: https://github.com/Kong/kong/blob/d65101fe80fd7ac9870a84d34d81bda8bcb461ac/kong/templates/nginx_kong_stream.lua#L3
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.
Okay.
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 add a test in https://github.com/apache/apisix/blob/master/t/cli/test_access_log.sh? Thanks.
apisix/cli/ngx_tpl.lua
Outdated
|
||
access_log {* stream.access_log *} main buffer=16384 flush=3; | ||
{% end %} | ||
open_log_file_cache max=1000 inactive=60; |
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.
We don't need it, as the log file name doesn't contain variable. There is only one open log file.
See https://nginx.org/en/docs/http/ngx_http_log_module.html#open_log_file_cache
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.
Ack. Thanks
apisix/cli/ngx_tpl.lua
Outdated
@@ -261,7 +271,7 @@ http { | |||
|
|||
access_log {* http.access_log *} main buffer=16384 flush=3; | |||
{% end %} | |||
open_file_cache max=1000 inactive=60; |
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.
open_file_cache is there: https://nginx.org/en/docs/http/ngx_http_core_module.html#open_file_cache
This directive is different from open_log_file_cache.
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.
Ack
conf/config-default.yaml
Outdated
@@ -174,6 +174,11 @@ nginx_config: # config for render the template to generate n | |||
# - TEST_ENV | |||
|
|||
stream: | |||
enable_access_log: true # enable access log or not, default true |
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.
The access_log in stream is false by default: https://nginx.org/en/docs/stream/ngx_stream_log_module.html#access_log
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.
fixed
conf/config-default.yaml
Outdated
@@ -174,6 +174,11 @@ nginx_config: # config for render the template to generate n | |||
# - TEST_ENV | |||
|
|||
stream: | |||
enable_access_log: true # enable access log or not, default true | |||
access_log: logs/access_stream.log | |||
access_log_format: "$remote_addr [$time_local] $status $bytes_sent $session_time $upstream_connect_time \"$protocol://$upstream_addr\"" |
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 can use Nginx's basic format in its example?
log_format basic '$remote_addr [$time_local] '
'$protocol $status $bytes_sent $bytes_received '
'$session_time';
See https://nginx.org/en/docs/stream/ngx_stream_log_module.html.
Kong also uses it too: https://github.com/Kong/kong/blob/d65101fe80fd7ac9870a84d34d81bda8bcb461ac/kong/templates/nginx_kong_stream.lua#L3
Done. Took a while to figure out how to set up mock upstream. |
d7c5b19
to
33c4a44
Compare
33c4a44
to
43ec2e5
Compare
t/cli/test_access_log.sh
Outdated
exit 1 | ||
fi | ||
|
||
make stop |
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.
We will clean up automatically, so there is no need to add it. See
Line 32 in 02d32c9
make stop || true |
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.
Awesome. Thanks for sharing the details. Updated.
What this PR does / why we need it:
closes #5400
Pre-submission checklist: