You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/01/20 07:27:41 UTC
[GitHub] [apisix] bisakhmondal commented on a change in pull request #5831: feat: add `file-logger` plugin
bisakhmondal commented on a change in pull request #5831:
URL: https://github.com/apache/apisix/pull/5831#discussion_r788432285
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message)
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
+ else
+ local ok, err = file:write(msg, '\n')
+ if not ok then
+ core.log.error("failed to write file: ", conf.path, ", error info: ", err)
+ else
+ file:flush()
+ end
+ file:close()
+ end
+end
Review comment:
Instead of opening a file handle for each time we dump a log, can we optimize it a bit by keeping the fd open and caching it so that it can be reused (maybe with a timeout, so that it doesn't remain open indefinitely [do we really care about it?])? Because you know, writing something is cheap but opening a file handler for every logs and moving the fd to the EOF takes some resources.
Maybe it's a premature optimization. @spacewander WDYT?
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
+end
+
+
+local function write_file_data(conf, log_message)
+ local msg = core.json.encode(log_message)
+ local file, err = io_open(conf.path, 'a+')
+
+ if not file then
+ core.log.error("failed to open file: ", conf.path, ", error info: ", err)
+ else
+ local ok, err = file:write(msg, '\n')
+ if not ok then
+ core.log.error("failed to write file: ", conf.path, ", error info: ", err)
+ else
+ file:flush()
+ end
+ file:close()
+ end
+end
+
+
+function _M.log(conf, ctx)
+ local metadata = plugin.plugin_metadata(plugin_name)
Review comment:
How about a gentle info log of the json serialized metadata after this line?
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,204 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
Review comment:
We can go with table-driven checks?
https://github.com/apache/apisix/blob/d1d16f403c3ded1dc7a9390d0252200afc2591cc/t/plugin/splunk-hec-logging.t#L46-L99
##########
File path: apisix/plugins/file-logger.lua
##########
@@ -0,0 +1,102 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local log_util = require("apisix.utils.log-util")
+local core = require("apisix.core")
+local plugin = require("apisix.plugin")
+local ngx = ngx
+local io_open = io.open
+
+
+local plugin_name = "file-logger"
+
+
+local schema = {
+ type = "object",
+ properties = {
+ path = {
+ type = "string"
+ },
+ },
+ required = {"path"}
+}
+
+
+local metadata_schema = {
+ type = "object",
+ properties = {
+ log_format = log_util.metadata_schema_log_format
+ }
+}
+
+
+local _M = {
+ version = 0.1,
+ priority = 399,
+ name = plugin_name,
+ schema = schema,
+ metadata_schema = metadata_schema
+}
+
+
+function _M.check_schema(conf, schema_type)
+ if schema_type == core.schema.TYPE_METADATA then
+ return core.schema.check(metadata_schema, conf)
+ end
+
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return nil, err
+ end
+ return log_util.check_log_schema(conf)
Review comment:
we don't need this validation as the current file-logger schema doesn't contains any expression like `include_resp_body_expr` or `include_req_body_expr`
##########
File path: t/plugin/file-logger.t
##########
@@ -0,0 +1,204 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (! $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if (! $block->no_error_log && ! $block->error_log) {
+ $block->set_value("no_error_log", "[error]");
+ }
+});
+
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: sanity
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema({
+ path = "file.log"
+ })
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+done
+
+
+
+=== TEST 2: path is missing check
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.file-logger")
+ local ok, err = plugin.check_schema()
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- no_error_log
+property "path" is required
+
+
+
+=== TEST 3: add plugin metadata
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/plugin_metadata/file-logger',
+ ngx.HTTP_PUT,
+ [[{
+ "log_format": {
+ "host": "$host",
+ "client_ip": "$remote_addr"
+ }
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(code..body)
+ }
+ }
+--- response_body
+201passed
+
+
+
+=== TEST 4: add plugin
+--- 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,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 5: verify plugin
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code = t("/hello", ngx.HTTP_GET)
+ local fd, err = io.open("file.log", 'r')
+ local msg
+
+ if fd then
+ msg = fd:read()
+ else
+ core.log.error("failed to open file: file.log, error info: ", err)
+ end
+ fd:close()
+
+ local new_msg = core.json.decode(msg)
+ if new_msg.client_ip == '127.0.0.1' and new_msg.route_id == '1'
+ and new_msg.host == '127.0.0.1'
+ then
+ msg = "write file log success"
+ ngx.status = code
+ ngx.say(msg)
+ end
+ }
+ }
+--- response_body
+write file log success
+
+
+
+=== TEST 6: failed to open the path
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "file-logger": {
+ "path": "/log/file.log"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ end
+
+ local code, messages = t("/hello", GET)
+ core.log.warn("messages: ", messages)
+ if code >= 300 then
+ ngx.status = code
+ end
+ }
+ }
+--- error_log
+failed to open file: /log/file.log, error info: /log/file.log: No such file or directory
Review comment:
In such a case, how about we try to create a file with the corresponding name. If that returns an error [maybe insufficient permission], we throw it away. Else it would not be a good user experience to create a file each time they create a log configuration. What do you think about it? Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org