Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: The / in the log will be added with an extra \ when enabled file-logger and sls-logger #7785

Closed
henkuaifacai opened this issue Aug 25, 2022 · 9 comments · Fixed by #8684
Assignees
Labels
good first issue Good for newcomers

Comments

@henkuaifacai
Copy link

Current Behavior

当时使用了file-logger和sls-logger会出现日志里的/会被加个\

Expected Behavior

希望能按正常的日志出输到文件和阿里云的sls

Error Logs

content:{"service_id":"","latency":0.99992752075195,"request":{"headers":{"if-modified-since":"Fri, 16 May 2014 15:12:48 GMT","upgrade-insecure-requests":"1","cache-control":"max-age=0","accept-encoding":"gzip, deflate","accept":"text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,/;q=0.8,application/signed-exchange;v=b3;q=0.9","user-agent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.121 Safari/537.36","accept-language":"zh-CN,zh;q=0.9","connection":"keep-alive","host":"60.test.com:9080","if-none-match":""53762af0-12e1""},"method":"GET","uri":"/","url":"http://60.test.com:9080/","querystring":{},"size":543},"client_ip":"192.168.139.1","upstream":"192.168.139.130:80","upstream_latency":1,"response":{"headers":{"date":"Wed, 24 Aug 2022 03:54:48 GMT","etag":""53762af0-12e1"","connection":"close","server":"APISIX/2.15.0","last-modified":"Fri, 16 May 2014 15:12:48 GMT"},"size":182,"status":304},"server":{"hostname":"Harbor","version":"2.15.0"},"apisix_latency":0,"route_id":"422421849201183291","start_time":1661313288377

Steps to Reproduce

只需要配置file-logger和sls-logger就会有这种情况

Environment

  • APISIX version (run apisix version):
  • Operating system (run uname -a):
  • OpenResty / Nginx version (run openresty -V or nginx -V):
  • etcd version, if relevant (run curl http://127.0.0.1:9090/v1/server_info):
  • APISIX Dashboard version, if relevant:
  • Plugin runner version, for issues related to plugin runners:
  • LuaRocks version, for installation issues (run luarocks --version):
@moonming moonming changed the title bug: 使用插件输出日志时会对‘/’进行转义,在其前面加上 ‘\’ bug: The / in the log will be added with an extra \ when enabled file-logger and sls-logger Aug 25, 2022
@moonming
Copy link
Member

image

@spacewander
Copy link
Member

The json library we used (lua-cjson) will escape / to \/, as this library chooses to be compatible with the behavior of PHP.

@henkuaifacai
Copy link
Author

The json library we used (lua-cjson) will escape / to \/, as this library chooses to be compatible with the behavior of PHP.

Is there any way to solve this problem

@Lewisyixin
Copy link

Lewisyixin commented Aug 25, 2022

我们自己根据filelogger插件做了一个根据域名拆分日志的插件,也遇到你这个问题了,我们是重新引用了openresty的json模块
openresty的json模块可以专门有一个参数可以关闭 '/' 前的''。

local json = require("cjson")
json.encode_escape_forward_slash(false)

apisix的json模块是自己稍微做了下封装,我测试过直接在core.json里开启这个参数也是能生效的,但是相当于全局使用的json都会被修改所以不太敢这么做。所以你们如果只是某个插件想去掉这个转义符,可以稍微修改下这个插件里的json模块引用。

@henkuaifacai
Copy link
Author

我们自己根据filelogger插件做了一个根据域名拆分日志的插件,也遇到你这个问题了,我们是重新引用了openresty的json模块 openresty的json模块可以专门有一个参数可以关闭 '/' 前的''。

local json = require("cjson")
json.encode_escape_forward_slash(false)

apisix的json模块是自己稍微做了下封装,我测试过直接在core.json里开启这个参数也是能生效的,但是相当于全局使用的json都会被修改所以不太敢这么做。所以你们如果只是某个插件想去掉这个转义符,可以稍微修改下这个插件里的json模块引用。

好的,我现在尝试一下,谢谢

@Lewisyixin
Copy link

Lewisyixin commented Aug 25, 2022

@spacewander
Also, could you please evaluate if json.encode_escape_forward_slash can be turned off in apisix.core.json?
I met this problem some days ago. #7606.
And backslash really make people headache especially when collecting or parsing logs.

By the way. I have found backslash in not caused by etcd but core.json in (#7606)

When I add json.encode_escape_forward_slash(false) in apisix.core.json and then create route by api, backslash is no longer exist. But Im not sure if this will bring problems.
image

@spacewander
Copy link
Member

@Lewisyixin
Thanks for your suggestion.
AFAIK, there is no feature that depends on the escape behavior.

Would you like to submit a PR for this? Thanks!

@henkuaifacai
Copy link
Author

@Lewisyixin Thank you. After the configuration, the \ is not available

@uran0sH
Copy link
Contributor

uran0sH commented Jan 12, 2023

I will work on it. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants