feat: provide skywalking logger plugin #5478

dmsolr · 2021-11-10T15:52:44Z

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? If it is not backward compatible, please discuss on the mailing list first

provide a new logger plugin to support log access log to SkyWalking in SkyWalking Log Format. If skywalking trace plugin is enabled or requests with SkyWalking Trace context header, this plugin will parse the trace context header and print trace context in log. So that the access log can relate to trace.

spacewander · 2021-11-11T03:39:58Z

apisix/plugins/skywalking-logger.lua

+        trace_context = {
+            traceId = base64.decode_base64url(ids[2]),
+            traceSegment = base64.decode_base64url(ids[3]),
+            spanId = tonumber(ids[4])


The span id is missing in the comment?

I update the comment. Currently, the 4th part, 3 means span id.

4. Parent span ID. Must be an integer. It begins with 0. This span ID points to the parent span in parent trace segment.

Ref SkyWalking Cross Process Propagation Headers Protocol

It would be better if you also add the link to the skywalking's doc.

spacewander · 2021-11-11T03:40:24Z

apisix/plugins/skywalking-logger.lua

+        local ids = ngx_re.split(headers["sw8"], '-')
+
+        trace_context = {
+            traceId = base64.decode_base64url(ids[2]),


Need to check the length of ids and the result of decode_base64url

apisix/plugins/skywalking-logger.lua

docs/en/latest/plugins/skywalking-logger.md

spacewander · 2021-11-11T03:46:51Z

apisix/plugins/skywalking-logger.lua

+    end
+
+    if not log_body.route_id then
+        log_body.route_id = "no-matched"


Is the no-matched necessary for the skywalking? If not, we can leave it empty.

Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>

moonming · 2021-11-11T05:42:44Z

@wu-sheng please take a look, and welcome feedback:)

wu-sheng · 2021-11-11T07:45:46Z

apisix/plugins/skywalking-logger.lua

+    local headers = ngx.req.get_headers()
+    if headers then
+        -- 1-TRACEID-SEGMENTID-3-PARENT_SERVICE-PARENT_INSTANCE-PARENT_ENDPOINT-IPPORT
+        local ids = ngx_re.split(headers["sw8"], '-')


Are you sure you want to do this? SkyWalking has tracer for APISIX, which has been integrated. Why don't you try to read it from the context?

Decoding the header is fine from tech perspective, but it could be a risk when one day, Lua agent supports namespace concept.

And all following decode(twice for trace id and span id) and splitting cost unnecessary resources.

I am considering about it still works when tracer plugin is not enabling.
If we give the version matching matric, it looks not very dangerous.

Your call, SkyWalking wouldn't change v3 format for a long time(already years, expect more) AFAIK. namespace would be an issue, but not very serious, just duplicate configurations.
I just think resource costing is very sensitive. And this is not that risk, as when the incompatible issue happens, it happens at the first time both plugin activated. So anyone will notice this immediately, they could do fix or disable one/both quickly.

Got it.
But now, skywalking-nginx-lua does not expose trace context. I would like to get merged this pr first. And keep going on polishing.
We need to enhancement skywalking-nginx-lua and release a new version. And then, I back to polishing this feature.

wu-sheng

General review from design perspective(nothing related to code details).

wu-sheng · 2021-11-11T07:46:40Z

apisix/plugins/skywalking-logger.lua

+    local headers = ngx.req.get_headers()
+    if headers then
+        -- 1-TRACEID-SEGMENTID-3-PARENT_SERVICE-PARENT_INSTANCE-PARENT_ENDPOINT-IPPORT
+        local ids = ngx_re.split(headers["sw8"], '-')


And all following decode(twice for trace id and span id) and splitting cost unnecessary resources.

wu-sheng · 2021-11-11T07:47:20Z

apisix/plugins/skywalking-logger.lua

+    }
+
+    if not stale_timer_running then
+        -- run the timer every 30 mins if any log is present


Every 30 mins? For?

I think this is a huge latency in low traffic scenario. No one is willing to wait for 30 mins for log collecting.

it sweeps the stale log data in every 30m.
When the data transport fails, the data will remain in memory. Therefore, regular cleaning is required.

I think this is not very general way to process logs. Usually, unless you have local cache or mmap(like SkyWalking Satellite), logs should be abandoned quickly once retry threshold reached.
The reason behind this logic is, observability presents a large data set but isn't always very useful, especially for logs. Metrics are small, and tolerable to keep latest values, but different from logs.

Make sense to me. @spacewander how do you think?

Err. @dmsolr's explain is incorrect. This timer doesn't clean up the stale log. It clean up the stale buffers when they are no longer holding the log.

If it is about clean up the stale buffers only, than, this is APISIX's call. Nothing relates to SkyWalking or observability preference.

spacewander · 2021-11-11T09:40:13Z

apisix/plugins/skywalking-logger.lua

+        -- 1-TRACEID-SEGMENTID-SPANID-PARENT_SERVICE-PARENT_INSTANCE-PARENT_ENDPOINT-IPPORT
+        local ids = ngx_re.split(sw_header, '-')
+        if #ids == 8 then
+            local trace_id, err = base64.decode_base64url(ids[2])


decode_base64url only returns str or nil

if so, we don't need to check the result of decode_base64url, right?

If I am correct, we should check if the trace_id is nil or not? The trace_id is required.

trace_context is optional. If trace_id is missing, it cannot relate to trace. But missing trace_id log can be found in log view.

Refer https://github.com/apache/skywalking-data-collect-protocol/blob/master/logging/Logging.proto#L99-L107

Currently, I prefer to collect the missing trace_context log.

spacewander · 2021-11-11T09:41:42Z

t/plugin/skywalking-logger.t

+    }
+_EOC_
+
+    $block->set_value("http_config", $http_config);


New tests should contain a common section, like

apisix/t/plugin/limit-count2.t

Line 38 in f06f6cc

if (!$block->request) {

There is no need to repeat them.

wu-sheng · 2021-11-11T11:09:21Z

docs/en/latest/plugins/skywalking-logger.md

@@ -0,0 +1,117 @@
+---
+title: http-logger


Is this the right title?

docs/en/latest/plugins/skywalking-logger.md

Co-authored-by: 吴晟 Wu Sheng <wu.sheng@foxmail.com>

wu-sheng

Good to me. Thanks.

membphis · 2021-11-12T09:35:47Z

t/plugin/skywalking-logger.t

+--- response_body
+opentracing
+--- error_log eval
+qr/.*\{\\\"json\\\":\\\"\{(\\\\\\\"\@timestamp\\\\\\\":\\\\\\\".*\\\\\\\"|\\\\\\\"client_ip\\\\\\\":\\\\\\\"127\.0\.0\.1\\\\\\\"|\\\\\\\"host\\\\\\\":\\\\\\\"localhost\\\\\\\"|\\\\\\\"route_id\\\\\\\":\\\\\\\"1\\\\\\\"|,){7}\}/


Not easy to read, can there be a more readable way? @dmsolr

this is raw skywalking log format.
Seem like have some approach to simplify it. I will have a try later.

wu-sheng · 2021-11-12T09:48:32Z

t/plugin/skywalking-logger.t

+
+=== TEST 7: test wrong trace context header
+--- request
+GET /opentracing


Ah, I suddenly noticed this, why /opentracing? Rather than /request.

dmsolr added 5 commits November 10, 2021 23:52

feat: provide skywalking logger plugin

65b696b

fix docs

c059f6b

fix code style

853f481

fix

2341e01

fix test

1d46469

spacewander reviewed Nov 11, 2021

View reviewed changes

dmsolr and others added 2 commits November 11, 2021 11:57

Apply suggestions from code review

dfafbdb

Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>

update

9132c65

wu-sheng reviewed Nov 11, 2021

View reviewed changes

spacewander reviewed Nov 11, 2021

View reviewed changes

wu-sheng reviewed Nov 11, 2021

View reviewed changes

dmsolr added 3 commits November 11, 2021 23:00

polish & fix wrong title

d5173c6

typo

674f499

typo

8847908

wu-sheng reviewed Nov 12, 2021

View reviewed changes

docs/en/latest/plugins/skywalking-logger.md Outdated Show resolved Hide resolved

dmsolr and others added 3 commits November 12, 2021 11:41

Update docs/en/latest/plugins/skywalking-logger.md

caf6896

Co-authored-by: 吴晟 Wu Sheng <wu.sheng@foxmail.com>

update cn docs

8dd3001

update cn docs

5403729

wu-sheng approved these changes Nov 12, 2021

View reviewed changes

spacewander approved these changes Nov 12, 2021

View reviewed changes

spacewander merged commit 5f6afdb into apache:master Nov 12, 2021

membphis reviewed Nov 12, 2021

View reviewed changes

wu-sheng reviewed Nov 12, 2021

View reviewed changes

dmsolr mentioned this pull request Feb 24, 2022

Plugin interaction and SkyWalking native log format with trace context #4232

Open

feat: provide skywalking logger plugin #5478

feat: provide skywalking logger plugin #5478

Conversation

dmsolr commented Nov 10, 2021

What this PR does / why we need it:

Pre-submission checklist:

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 11, 2021

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

wu-sheng 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

wu-sheng Nov 11, 2021 • edited Loading

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

wu-sheng 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

wu-sheng Nov 11, 2021 •

edited

Loading