feat: development of Loggly logging plugin by bisakhmondal · Pull Request #6113 · apache/apisix

bisakhmondal · 2022-01-14T18:03:52Z

Signed-off-by: Bisakh Mondal [email protected]

What this PR does / why we need it:

#6112

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

bisakhmondal · 2022-01-14T19:14:34Z

The current logger implementation

membphis · 2022-01-15T09:25:34Z

-local _M = { version = 0.1 }
+local _M = {
+    version = 0.1,
+    severity = Severity


why we need to update this code?

one PR for one thing. so I think we need to revert this PR

Hi, thanks for the code review and the concern. The current PR need to access the rfc5424 severity map to decode alert status into integer numbers.

local message = {
-- facility LOG_USER - random user level message
"<".. tostring(8 + severity[conf.severity]) .. ">1", -- 1
timestamp, -- timestamp
ctx.var.host or "-", -- hostname
"apisix", -- appname
ctx.var.pid, -- proc-id
"-", -- msgid
"[" .. conf.customer_token .. "@41058 " .. tab_concat(taglist, " ") .. "]",
json_str
}

I have made the field exported here from that module. Okay, reverting the changes back with a local copy of that severity map inside the plugin. Thank you

Signed-off-by: Bisakh Mondal <[email protected]>

membphis

need to update the README.md

spacewander · 2022-01-20T05:14:01Z

+                    "debug", "info", "notice", "warning", "err", "crit", "alert", "emegr"},
+            description = "base severity log level",
+        },
+        include_req_body = {type = "boolean", default = false},


Could we refactor it to avoid adding the same group of fields to each logger?

And you miss to call log_util.collect_body(conf, ctx).

Could we refactor it to avoid adding the same group of fields to each logger?

Yes, similar to what we did to batchprocessor. Let's handle that in the next PR.

RE collect_body: Good catch. Adding, Thanks

spacewander · 2022-01-20T05:15:43Z

+        entry = log_util.get_full_log(ngx, conf)
+    end
+
+    if conf.prefer_name then


It seems we don't need this as we support customizing format?

spacewander · 2022-01-20T05:22:11Z

+    local metadata = plugin.plugin_metadata(plugin_name)
+    local entry
+
+    if metadata and metadata.value.log_format


Let's refactor it in the next PR

bisakhmondal · 2022-01-20T05:44:16Z

need to update the README.md

Yes definitely. Do you want this to be handled in this PR or as usual a small PR with a readme update?
Thanks.

membphis · 2022-01-20T09:34:58Z

+local function handle_log(entries)
+    local ok, err
+    for i = 1, #entries do
+        ok, err = send_data_over_udp(entries[i])


do we need to check the return value?

Yes, if in case, the batch processor fails to process the entries (network error or something like that) the batch will be retried with a retry policy.
ref:

apisix/apisix/utils/batch-processor.lua

Lines 66 to 89 in e9fab8b

function execute_func(premature, self, batch)

if premature then

return

end

local ok, err = self.func(batch.entries, self.batch_max_size)

if not ok then

core.log.error("Batch Processor[", self.name,

"] failed to process entries: ", err)

batch.retry_count = batch.retry_count + 1

if batch.retry_count <= self.max_retry_count then

schedule_func_exec(self, self.retry_delay,

batch)

else

core.log.error("Batch Processor[", self.name,"] exceeded ",

"the max_retry_count[", batch.retry_count,

"] dropping the entries")

end

return

end

core.log.debug("Batch Processor[", self.name,

"] successfully processed the entries")

end

However, seeing your comment make me think that we can migrate to a fail-fast approach. It has other benefits.

for i = 1, #entries do local ok, err = send_data_over_udp(entries[i]) if not ok then return false, err end end return true

Thank you.

Look like we need to remove the sent entries? Otherwise, they will be retried.

Yes, that's an issue here. The way batchprocessor is written, it is meant for consuming each batch in one go (transactional). But in the case of loggly, only bulk events can be sent through HTTP endpoints. So I can think of three potential resolution

if err and i==1 -> then return false else true | if some entries has been sent that's ok. we are loosing the rest of the entries. [some entries will be dropped]

use the current behaviour. where some entries might be retried again [duplocation].

Update batch processor to have this fine-grained control

apisix/apisix/utils/batch-processor.lua

Lines 66 to 89 in a813b5a

function execute_func(premature, self, batch)

if premature then

return

end

local ok, err = self.func(batch.entries, self.batch_max_size)

if not ok then

core.log.error("Batch Processor[", self.name,

"] failed to process entries: ", err)

batch.retry_count = batch.retry_count + 1

if batch.retry_count <= self.max_retry_count then

schedule_func_exec(self, self.retry_delay,

batch)

else

core.log.error("Batch Processor[", self.name,"] exceeded ",

"the max_retry_count[", batch.retry_count,

"] dropping the entries")

end

return

end

core.log.debug("Batch Processor[", self.name,

"] successfully processed the entries")

end

update

local ok, err = self.func(batch.entries, self.batch_max_size) -- to this local ok, err, n = self.func(batch.entries, self.batch_max_size) -- shorten self.entries based on n

which one do you think would be good? cc @spacewander

here n denotes the number of processed entries, and the rest of the entries should be retried.

cc @spacewander

spacewander · 2022-01-21T01:40:44Z

need to update the README.md

Yes definitely. Do you want this to be handled in this PR or as usual a small PR with a readme update? Thanks.

I suggest doing it in the next PR.

spacewander · 2022-01-25T03:11:45Z

+--- grep_error_log eval
+qr/message received: [ -~]+/
+--- grep_error_log_out eval
+qr/message received: <10>1 [\d\-T:.]+Z [\d.]+ apisix [\d]+ - \[token-1\@41058 ] \{"apisix_latency":[\d.]*,"client_ip":"127\.0\.0\.1","latency":[\d.]*,"request":\{"headers":\{"content-type":"application\/x-www-form-urlencoded","host":"127\.0\.0\.1:1984","user-agent":"lua-resty-http\/[\d.]* \(Lua\) ngx_lua\/[\d]*"\},"method":"GET","querystring":\{\},"size":[\d]+,"uri":"\/opentracing","url":"http:\/\/127\.0\.0\.1:1984\/opentracing"\},"response":\{"headers":\{"connection":"close","content-type":"text\/plain","server":"APISIX\/[\d.]+","transfer-encoding":"chunked"\},"size":[\d]*,"status":200\},"route_id":"1","server":\{"hostname":"[ -~]*","version":"[\d.]+"\},"service_id":"","start_time":[\d]*,"upstream":"127\.0\.0\.1:1982","upstream_latency":[\d]*\}/


We need to configure include_resp_body_expr & include_resp_body to get the body.

Done. Also added case for metadata.log_format

bisakhmondal added the enhancement New feature or request label Jan 14, 2022

membphis requested a review from shuaijinchao January 15, 2022 09:24

membphis reviewed Jan 15, 2022

View reviewed changes

juzhiyuan requested a review from totemofwolf January 16, 2022 09:12

bisakhmondal added 6 commits January 19, 2022 12:15

working implementation of loggly plugin

2f07687

Signed-off-by: Bisakh Mondal <[email protected]>

fixed formatting issue

376a19e

Signed-off-by: Bisakh Mondal <[email protected]>

revert rfc5424 module

2ab6f1f

Signed-off-by: Bisakh Mondal <[email protected]>

loggly tests

b885086

Signed-off-by: Bisakh Mondal <[email protected]>

loggly documentation

7fa9cc1

Signed-off-by: Bisakh Mondal <[email protected]>

remove latency details method from recalling

a3e4559

Signed-off-by: Bisakh Mondal <[email protected]>

bisakhmondal force-pushed the plugin/logger/loggly branch from 6b8fe38 to a3e4559 Compare January 19, 2022 06:47

bisakhmondal marked this pull request as ready for review January 19, 2022 06:56

bisakhmondal requested a review from spacewander January 19, 2022 12:03

update with log_format

3976fa4

Signed-off-by: Bisakh Mondal <[email protected]>

membphis reviewed Jan 20, 2022

View reviewed changes

spacewander reviewed Jan 20, 2022

View reviewed changes

applied code review suggestions

9f4f17a

membphis reviewed Jan 20, 2022

View reviewed changes

fail fast batch processor exec

7f71ddc

spacewander reviewed Jan 21, 2022

View reviewed changes

Comment thread t/plugin/loggly.t

shuaijinchao reviewed Jan 21, 2022

View reviewed changes

Comment thread apisix/plugins/loggly.lua

bisakhmondal added 3 commits January 24, 2022 12:34

new test

1086d94

Merge branch 'master' into plugin/logger/loggly

b432d13

streamline mock layer4 methods

f708656

bisakhmondal requested review from shuaijinchao and spacewander January 24, 2022 18:24

bisakhmondal mentioned this pull request Jan 24, 2022

docs: add solarwinds loggly to readme #6191

Merged

4 tasks

spacewander reviewed Jan 25, 2022

View reviewed changes

bisakhmondal added 2 commits January 25, 2022 12:10

tests with log_format and include_resp_body_expr

0705084

lint fix

dc65f2c

spacewander approved these changes Jan 25, 2022

View reviewed changes

bisakhmondal mentioned this pull request Jan 25, 2022

feat(batchprocessor): support partial consumption of batch entries #6203

Merged

4 tasks

shuaijinchao approved these changes Jan 26, 2022

View reviewed changes

membphis approved these changes Jan 26, 2022

View reviewed changes

spacewander merged commit 53420f6 into apache:master Jan 26, 2022

	function execute_func(premature, self, batch)
	if premature then
	return
	end

	local ok, err = self.func(batch.entries, self.batch_max_size)
	if not ok then
	core.log.error("Batch Processor[", self.name,
	"] failed to process entries: ", err)
	batch.retry_count = batch.retry_count + 1
	if batch.retry_count <= self.max_retry_count then
	schedule_func_exec(self, self.retry_delay,
	batch)
	else
	core.log.error("Batch Processor[", self.name,"] exceeded ",
	"the max_retry_count[", batch.retry_count,
	"] dropping the entries")
	end
	return
	end

	core.log.debug("Batch Processor[", self.name,
	"] successfully processed the entries")
	end

Uh oh!

Conversation

bisakhmondal commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

bisakhmondal commented Jan 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

membphis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bisakhmondal Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bisakhmondal commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacewander commented Jan 21, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bisakhmondal commented Jan 14, 2022 •

edited

Loading

bisakhmondal Jan 20, 2022 •

edited

Loading

bisakhmondal commented Jan 20, 2022 •

edited

Loading