feat: Support plugin for "aliyun" log service #2177

gy09535 · 2020-09-07T08:43:08Z

What this PR does / why we need it:

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?

…9535/incubator-apisix into feature/support-alicloud-sls * 'feature/support-alicloud-sls' of https://github.com/gy09535/incubator-apisix: doc: fixed typo in `health-check.md` (#2200) bugfix: grpc-transcode plugin converts http/json parameters abnormally (#2139) chore: add string utility for simplicity and correctness (#2181) improve: cache parsed certs and pkeys to LRU cache (#2163) bugfix: serverless plugin not work in header_filter phase (#2148)

apisix/plugins/slslog/rfc5424.lua

conf/config-default.yaml

moonming · 2020-09-14T08:47:21Z

t/plugin/sls-logger.t

+
+
+
+=== TEST 4: add plugin


Is there a way to verify the response of Alibaba Cloud?
If not, we can mock Alibaba Cloud

The aliyun log service can not response anything. I can just send it to collector node and then verify it on log service UI, for collector not response anything.I think if the message is send successful with no error log, sending message is success.

we can add a comment in the source code, and mark this plugin is experiment. we'll remove this after the community confirm this plugin is stable

moonming · 2020-09-14T08:53:55Z

@membphis @nic-chen please take a look

moonming · 2020-09-14T08:56:29Z

@gy09535 Apache APISIX already has the syslog plugin, can these two plugins be combined into one?

gy09535 · 2020-09-14T13:43:42Z

@gy09535 Apache APISIX already has the syslog plugin, can these two plugins be combined into one?

For combined into one I think the aliyun has some special config, we should has different plugin to config it and I reference tcp logger plugin to implement it, I think I can refactor tcp logger and sls logger plugin , change the common code into one module, For sys-logger plugin I think some code is duplicate , for example the batch processor has retry the "resty.logger.socket" also has retry etc. so I prefer use cosocket .

moonming · 2020-09-24T04:53:13Z

@liuhengloveyou please take a look at this PR

liuhengloveyou · 2020-09-27T06:12:23Z

@liuhengloveyou please take a look at this PR

working on it.

apisix/plugins/sls-logger.lua

gy09535 · 2020-10-16T09:58:11Z

any update?

membphis · 2020-10-16T13:37:27Z

any update?

you can rebase your branch first. conflicted

membphis · 2020-10-16T14:10:33Z

@gy09535 I think we need to way to confirm this plugin can work fine with aliyun. if we can check this in the test, it should be great.

change space

Makefile

membphis · 2020-11-05T13:25:36Z

apisix/plugins/sls-logger.lua

+
+    if not sock then
+        err_msg = "failed to init the socket" .. soc_err
+        core.log.error(err_msg)


we should avoid to concat string in Lua land, it'll make the GC busy.

we should use this style:
core.log.error("failed to init the socket", soc_err)

please fix the similar points.

membphis · 2020-11-05T13:27:23Z

apisix/plugins/sls-logger.lua

+        return
+    end
+
+    local rf5424_data = rf5424.encode("SYSLOG", "INFO", ngx.var.host


in APISIX, we shuold use ctx.var.**** to fetch nginx variable.

here is an example: https://github.com/apache/apisix/blob/master/apisix/plugins/limit-count.lua#L166

membphis · 2020-11-05T13:27:53Z

apisix/plugins/slslog/rfc5424.lua

+    end
+
+    return string_format(rfc5424_format, pri, t, hostname, appname, pid, project,
+     logstore, access_key_id, access_key_secret, msg)


bad indentation

I do not think is my format is right, I reference this doc: https://github.com/apache/apisix/blob/master/CODE_STYLE.md#fromHistory

gy09535 · 2020-11-09T06:23:46Z

please review

membphis · 2020-11-09T14:26:25Z

apisix/plugins/sls-logger.lua

+        ok, err = sock:setkeepalive(120 * 1000, 20)
+        if not ok then
+            can_close = true
+            core.log.warn("failed to set socket keepalive: host[" .. route_conf.host


bad performance, avoid concat string in Lua land.

core.log.warn("xxxx [", conf.host, "] port[", ...), please use this way. and please update other similar points

membphis · 2020-11-09T14:27:01Z

apisix/plugins/sls-logger.lua

+        end
+
+        data = data .. entry.data
+        core.log.info(entry.data)


need a simple description?

membphis · 2020-11-09T14:27:45Z

apisix/plugins/sls-logger.lua

+    end
+
+    local rf5424_data = rf5424.encode("SYSLOG", "INFO", ctx.var.host,"apisix",
+                                      ngx.var.pid, conf.project, conf.logstore,


please use ctx.var.*** instead of ngx.var.***

ctx.var.*** is better performance

please fix other similar poitns

membphis · 2020-11-09T14:29:04Z

apisix/plugins/slslog/rfc5424.lua

+local string_format = string.format
+local rfc5424_timestamp_format = "!%Y-%m-%dT%H:%M:%S.000Z"
+local rfc5424_format = "<%d>1 %s %s %s %d - [logservice project=\"%s\" logstore=\"%s\"" ..
+                    " access-key-id=\"%s\" access-key-secret=\"%s\"] %s\n"


bad indentation

membphis · 2020-11-09T14:30:21Z

doc/plugins/sls-logger.md

+```shell
+curl http://127.0.0.1:9080/apisix/admin/routes/5 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+      "plugins": {


the indentation should be 4 spaces here. please update it

membphis · 2020-11-09T14:31:15Z

doc/plugins/sls-logger.md

+
+* check log in ali cloud log service
+
+![](../images/plugin/sls-logger-1.png)


[****] please set a title, when failed to load the image, it will show the title

membphis · 2020-11-09T14:31:30Z

doc/plugins/sls-logger.md

+```shell
+$ curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d value='
+{
+    "methods": ["GET"],


remove this useless line

and please update the Chinese doc too

moonming · 2020-11-09T15:02:27Z

@membphis please check test cases and feature first, we can put code style issues at the end.

membphis · 2020-11-09T15:14:50Z

@membphis please check test cases and feature first, we can put code style issues at the end.

I read the doc and feature before, they are fine. sure about this.

gy09535 · 2020-11-10T03:06:28Z

style fixed.

gy09535 · 2020-11-12T08:46:11Z

Today Ido a test , I find using string format can costing more cpu , after I change it to string link, it costing little. Here is two picture is before change and after change .

Test Script

./wrk -c 1000 -t 10 -d 60s --latency   http://192.168.100.96:9080/whoami

Before

After

membphis

LGTM, except minor doc and style things

membphis · 2020-11-15T03:14:26Z

apisix/plugins/slslog/rfc5424.lua

+local LOG_LOCAL7    = 23      --  reserved for local use
+
+local Facility = {
+    ["KERN"] = LOG_KERN,


code style:

change to KERN = LOG_KERN, which is clearer

membphis · 2020-11-15T03:15:02Z

doc/plugins/sls-logger.md

+            "access_key_id": "your_access_key_id",
+            "access_key_secret": "your_access_key_secret",
+            "timeout": 30000
+            }


bad indentation

membphis · 2020-11-15T03:15:07Z

doc/plugins/sls-logger.md

+            "access_key_secret": "your_access_key_secret",
+            "timeout": 30000
+            }
+       },


membphis · 2020-11-15T03:17:11Z

t/plugin/sls-logger.t

+
+
+
+=== TEST 4: add plugin


we can add a comment in the source code, and mark this plugin is experiment. we'll remove this after the community confirm this plugin is stable

gy09535 · 2020-11-18T06:03:56Z

Please review CC @moonming

membphis

LGTM

init sls plugin

331f4c5

gy09535 closed this Sep 7, 2020

gy09535 added 3 commits September 9, 2020 22:18

add sls log for ali cloud

7f0b9c7

add. socket keep alive

eb35bef

add test case

89a97b8

gy09535 reopened this Sep 10, 2020

gy09535 added 3 commits September 10, 2020 15:42

change code style

9a1609e

add license

d3be3f5

add doc

e329b81

gy09535 changed the title ~~WIP:(feature) support send log to aliyun log service~~ feat: support send log to aliyun log service Sep 10, 2020

gy09535 added 3 commits September 10, 2020 21:47

Merge branch 'master' into feature/support-alicloud-sls

e0812da

add sls logger debug

1b01e3c

gy09535 changed the title ~~feat: support send log to aliyun log service~~ feat: support log for aliyun log service Sep 11, 2020

gy09535 changed the title ~~feat: support log for aliyun log service~~ feat: support logger for aliyun log service Sep 11, 2020

gy09535 mentioned this pull request Sep 14, 2020

feat: how use apisix send log to aliyun sls #2169

Closed

moonming reviewed Sep 14, 2020

View reviewed changes

moonming changed the title ~~feat: support logger for aliyun log service~~ feat: support plugin for aliyun log service Sep 14, 2020

moonming changed the title ~~feat: support plugin for aliyun log service~~ feature: support plugin for aliyun log service Sep 14, 2020

gy09535 mentioned this pull request Sep 25, 2020

bugfix: return right response code when http_to_https enabled in redirect plugin. #2311

Merged

4 tasks

liuhengloveyou reviewed Sep 27, 2020

View reviewed changes

apisix/plugins/sls-logger.lua Show resolved Hide resolved

liuhengloveyou reviewed Sep 27, 2020

View reviewed changes

apisix/plugins/sls-logger.lua Show resolved Hide resolved

gy09535 added 3 commits November 5, 2020 18:45

Update Makefile

34f0a35

change space

Update Makefile

259ddc2

change space

Merge branch 'master' into feature/support-alicloud-sls

b0350d1

membphis requested changes Nov 5, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

membphis requested changes Nov 5, 2020

View reviewed changes

gy09535 added 5 commits November 6, 2020 10:24

fix sls logger format

9374f48

change line format

b4d7199

change long line

c1a5bc7

change ctx host

e0b00d1

Merge branch 'master' into feature/support-alicloud-sls

b00a7f8

membphis reviewed Nov 9, 2020

View reviewed changes

gy09535 added 2 commits November 10, 2020 10:45

fix code style

c4f1ca6

change markdown title

65879a5

replace format string with related string

00d1fbb

remove useless var

dbf253f

gy09535 requested a review from membphis November 13, 2020 11:24

membphis approved these changes Nov 15, 2020

View reviewed changes

gy09535 added 2 commits November 16, 2020 10:22

fix style

6471993

Merge branch 'master' into feature/support-alicloud-sls

f288779

gy09535 requested a review from moonming November 16, 2020 11:31

membphis approved these changes Nov 18, 2020

View reviewed changes

moonming approved these changes Nov 18, 2020

View reviewed changes

membphis merged commit 5b39602 into apache:master Nov 18, 2020

moonming added this to the 2.1 milestone Nov 24, 2020


		* check log in ali cloud log service

		![](../images/plugin/sls-logger-1.png)

feat: Support plugin for "aliyun" log service #2177

feat: Support plugin for "aliyun" log service #2177

Conversation

gy09535 commented Sep 7, 2020 • edited Loading

What this PR does / why we need it:

Pre-submission checklist:

Choose a reason for hiding this comment

gy09535 Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moonming commented Sep 14, 2020

moonming commented Sep 14, 2020

gy09535 commented Sep 14, 2020 • edited Loading

moonming commented Sep 24, 2020

liuhengloveyou commented Sep 27, 2020

gy09535 commented Oct 16, 2020

membphis commented Oct 16, 2020

membphis commented Oct 16, 2020

Choose a reason for hiding this comment

gy09535 Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gy09535 Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gy09535 Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gy09535 commented Nov 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moonming commented Nov 9, 2020

membphis commented Nov 9, 2020

gy09535 commented Nov 10, 2020

gy09535 commented Nov 12, 2020 • edited Loading

Test Script

Before

After

membphis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gy09535 commented Nov 18, 2020

membphis left a comment

Choose a reason for hiding this comment

gy09535 commented Sep 7, 2020 •

edited

Loading

gy09535 Sep 14, 2020 •

edited

Loading

gy09535 commented Sep 14, 2020 •

edited

Loading

gy09535 Nov 6, 2020 •

edited

Loading

gy09535 Nov 6, 2020 •

edited

Loading

gy09535 Nov 6, 2020 •

edited

Loading

gy09535 commented Nov 12, 2020 •

edited

Loading