feat: add inspect plugin by kingluo · Pull Request #8400 · apache/apisix

kingluo · 2022-11-25T03:42:20Z

Description

It's useful to set arbitrary breakpoint in any lua file to inspect the context information, e.g. print local variables if some condition satisfied.

In this way, you don't need to modify the source code of your project, and just get diagnose information on demand, i.e. dynamic logging.

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

moonming

why we need this plugin? And I think it's diagnostic not inspect

kingluo · 2022-11-25T09:28:59Z

why we need this plugin? And I think it's diagnostic not inspect

In my opinion, Inspect means we could set breakpoints to get context information from any line of the code, while diagnostic means the action you handle the issue.
After inspect gathers all information you need, you could diagnose where could be the source of the issue.
The plugin could not determine the reason but it could help you to locate the reason.
So I think inspect is a more accurate name for this plugin.

And, the naming comes from the design document, and the document was publicized for more than 3 weeks.

…n blacklist for jit forever

spacewander · 2022-12-08T06:59:54Z

+    if not f then
+        return false, err
+    end
+    local code = f:read("*all")


Let's do some err check?

What about your opinion on this?

soulbird · 2022-12-08T07:26:40Z

+        delay = attr.delay
+        hooks_file = attr.hooks_file
+    end
+    ngx.log(ngx.INFO, "delay=", delay, ", hooks_file=", hooks_file)


core.log.info

spacewander · 2022-12-12T06:37:08Z

 	$(ENV_INSTALL) apisix/plugins/ip-restriction/*.lua $(ENV_INST_LUADIR)/apisix/plugins/ip-restriction/

+	$(ENV_INSTALL) -d $(ENV_INST_LUADIR)/apisix/inspect
+	$(ENV_INSTALL) apisix/inspect/*.lua $(ENV_INST_LUADIR)/apisix/inspect/


Should be moved under $(ENV_INSTALL) apisix/include/

spacewander · 2022-12-12T06:38:18Z

+    if not f then
+        return false, err
+    end
+    local code = f:read("*all")


What about your opinion on this?

kingluo · 2022-12-12T08:04:56Z

@spacewander Add nil case handling already. GitHub failed to add an outdated flag here.

https://github.com/kingluo/apisix/blob/f4fe75f0f3855883b46a61d078018a4396639f13/apisix/inspect/init.lua#L46-L50

tzssangglass · 2022-12-12T10:14:11Z

+    end
+end
+
+local function hook(evt, arg)


evt unused in hook function?

tzssangglass · 2022-12-12T10:21:08Z

+        for _, hook in ipairs(dbg.hooks()) do
+            table_insert(hooks, hook.key)
+        end
+        core.log.info("set hooks: err=", err, ", hooks=", core.json.encode(hooks))


Suggested change

core.log.info("set hooks: err=", err, ", hooks=", core.json.encode(hooks))

core.log.info("set hooks: err: ", err, ", hooks: ", core.json.delay_encode(hooks))

is better?

tzssangglass · 2022-12-12T10:23:00Z

+    if not func then
+        return false, err
+    end
+    func()


do we need use pcall(func()) here?

Already did so in the outer function.

spacewander · 2022-12-13T06:41:18Z

+    local code = f:read("*all")
+    f:close()
+    if code == nil then
+        return false, "cannot read hooks file"


We can improve the error message here by adding the err returned from the f:read above.

https://github.com/LuaJIT/LuaJIT/blob/8625eee71f16a3a780ec92bc303c17456efc7fb3/src/lib_aux.c#L39
The second returned value is a string describing the err.

spacewander · 2022-12-13T06:42:08Z

@spacewander Add nil case handling already. GitHub failed to add an outdated flag here.

kingluo/apisix@f4fe75f/apisix/inspect/init.lua#L46-L50

I see. Thanks for your clarification.

membphis · 2022-12-15T01:50:15Z

+  - APISIX
+  - Plugin
+  - inspect
+  - inspect


we should more this line

membphis · 2022-12-15T01:50:36Z

+
+## Description
+
+It's useful to set arbitrary breakpoint in any lua file to inspect the context information,


lua -> Lua

membphis · 2022-12-15T01:52:30Z

+
+## Features
+
+* set breakpoint at any position


Capitalized?

membphis · 2022-12-15T01:52:36Z

+## Features
+
+* set breakpoint at any position
+* dynamic breakpoint


moonming · 2022-12-15T02:19:54Z

+-- limitations under the License.
+--
+local core = require("apisix.core")
+local dbg = require("apisix.inspect.dbg")


What‘s dbg means?

Abbreviation for "debug". Also to avoid conflict with the built-in Lua debug module.

moonming · 2022-12-15T02:20:20Z

 #  redirect:
 #    https_port: 8443   # the default port for use by HTTP redirects to HTTPS
+  inspect:
+    delay: 3


3s or 3 mins?

3 seconds, I would add a comment for this line.

moonming · 2022-12-15T08:08:23Z

 #    https_port: 8443   # the default port for use by HTTP redirects to HTTPS
+  inspect:
+    delay: 3            # in seconds
+    hooks_file: "/var/run/apisix_inspect_hooks.lua"


What is this file for? Has Lua code in it?

@moonming yes, this hooks file is used to define breakpoints, which itself is lua file. When the administrator needs to set breakpoints to inspect something, he would edit this file, or link another file to this path.

Normally this file is non-exist. It's only available when you need to set breakpoints. Please refer to the plugin doc.

There are security risks.
This Lua code file must be in the APISIX directory, and users who do not have write permissions to the APISIX directory cannot write or modify this file.

moonming · 2022-12-15T08:26:50Z

+# create a hooks file to set a test breakpoint
+# Note that the breakpoint is associated with the line number,
+# so if the Lua code changes, you need to adjust the line number in the hooks file
+cat <<EOF >/tmp/hooks.lua


Do not use the /tmp directory, especially for documentation

ok, I would change it.

moonming · 2022-12-15T08:28:00Z

 #    https_port: 8443   # the default port for use by HTTP redirects to HTTPS
+  inspect:
+    delay: 3            # in seconds
+    hooks_file: "/var/run/apisix_inspect_hooks.lua"


There are security risks.
This Lua code file must be in the APISIX directory, and users who do not have write permissions to the APISIX directory cannot write or modify this file.

kingluo · 2022-12-15T08:34:56Z

@moonming /var/run/apisix_inspect_hooks.lua is only writeable for the root user. So I think no risk here. But it's up to you. Do you want to change it to /usr/local/openresty/apisix_inspect_hooks.lua?

kingluo added 7 commits November 24, 2022 17:12

feat: add inspect plugin

0086938

adapt APISIX coding style

485956f

fix lint

25a8147

fix lint

e7407a7

fix PR

bcbfb26

fix indent

0d8204e

fix PR

a444068

moonming reviewed Nov 25, 2022

View reviewed changes

moonming requested changes Nov 25, 2022

View reviewed changes

Comment thread t/plugin/inspect.t Outdated

kingluo marked this pull request as draft November 25, 2022 09:00

kingluo added 11 commits November 29, 2022 17:58

add new test cases

f113090

fix PR

2d16a2c

add default timeout

1767979

bugfix: jit off after flush, otherwise the hooked function would be i…

c9dca47

…n blacklist for jit forever

add test cases to test jit cache

d1099c0

Create inspect.md

72dd417

Add files via upload

cfd6e72

fix doc

4156700

fix doc

4d21630

Update inspect.md

0f3dcb1

Update inspect.md

a19b580

kingluo marked this pull request as ready for review December 7, 2022 11:00

spacewander reviewed Dec 8, 2022

View reviewed changes

fix PR

2a4aac6

soulbird reviewed Dec 9, 2022

View reviewed changes

kingluo added 3 commits December 9, 2022 11:23

fix PR

cf0038c

fix PR

9c4c58c

fix PR

9792e4c

spacewander reviewed Dec 12, 2022

View reviewed changes

fix PR

f4fe75f

tzssangglass reviewed Dec 12, 2022

View reviewed changes

spacewander reviewed Dec 13, 2022

View reviewed changes

fix PR

483ff23

spacewander previously approved these changes Dec 14, 2022

View reviewed changes

soulbird previously approved these changes Dec 15, 2022

View reviewed changes

membphis reviewed Dec 15, 2022

View reviewed changes

membphis requested review from leslie-tsang and moonming December 15, 2022 01:57

moonming reviewed Dec 15, 2022

View reviewed changes

fix PR

1916a2e

kingluo dismissed stale reviews from soulbird and spacewander via 1916a2e December 15, 2022 07:40

kingluo requested review from moonming and removed request for leslie-tsang December 15, 2022 07:59

moonming reviewed Dec 15, 2022

View reviewed changes

moonming requested changes Dec 15, 2022

View reviewed changes

fix PR

6245336

moonming approved these changes Dec 15, 2022

View reviewed changes

soulbird approved these changes Dec 16, 2022

View reviewed changes

leslie-tsang approved these changes Dec 16, 2022

View reviewed changes

tzssangglass merged commit 8486800 into apache:master Dec 16, 2022

	core.log.info("set hooks: err=", err, ", hooks=", core.json.encode(hooks))
	core.log.info("set hooks: err: ", err, ", hooks: ", core.json.delay_encode(hooks))


		## Description

		It's useful to set arbitrary breakpoint in any lua file to inspect the context information,

Uh oh!

Conversation

kingluo commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

moonming left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kingluo commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

kingluo commented Dec 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacewander commented Dec 13, 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!

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!

kingluo Dec 15, 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!

kingluo Dec 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingluo commented Dec 15, 2022

Uh oh!

Reviewers

kingluo commented Nov 25, 2022 •

edited

Loading

kingluo commented Nov 25, 2022 •

edited

Loading

kingluo Dec 15, 2022 •

edited

Loading

kingluo Dec 15, 2022 •

edited

Loading