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

feat: file logger plugin support response body in variable #8711

Merged

Conversation

mscb402
Copy link
Contributor

@mscb402 mscb402 commented Jan 19, 2023

Description

We need to add a new var resp_body to let the user put it in a log format which let the user use the response in the way there want.

Fixes #8705

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)

@mscb402 mscb402 marked this pull request as ready for review January 28, 2023 06:19
@@ -0,0 +1,124 @@
#
Copy link
Member

@spacewander spacewander Jan 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move these tests under https://github.com/apache/apisix/blob/master/t/plugin/file-logger2.t? That file is still small and doesn't have special setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to file-logger2

@@ -59,6 +59,9 @@ local _M = {
metadata_schema = metadata_schema
}

core.ctx.register_var("resp_body", function(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to put it in https://github.com/apache/apisix/blob/master/apisix/core/ctx.lua so other loggers can use it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this var should be put into the ctx.lua file. Because the value of resp_body comes from ctx.resp_body, which is set by file-logger when include_resp_body = true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only one logger plugin will support recording the response body. file-logger.lua is not an appropriate place.

| path | string | True | Log file path. |
| include_resp_body | boolean | False | When set to `true` includes the response body in the log file. |
| include_resp_body | boolean | False | When set to `true` includes the response body in the log file. If format is set, you can use the variable $resp_body to get the response body. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to put it in https://github.com/apache/apisix/blob/master/docs/en/latest/apisix-variable.md, instead of modifying each logger's doc.
Note that we need to mean that this variable is only for logger and requires the logger to have a special configuration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1.

@@ -213,6 +213,10 @@ do
route_name = true,
service_id = true,
service_name = true,
resp_body = function(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-- sort in alphabetical

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -213,6 +213,10 @@ do
route_name = true,
service_id = true,
service_name = true,
resp_body = function(ctx)
-- only work when file-logger set include_resp_body = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is incorrect. See #8711 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mscb402 mscb402 requested review from spacewander and tokers and removed request for spacewander and tokers February 3, 2023 05:43
@@ -49,5 +49,6 @@ additional variables.
| service_name | core | Name of Service. | |
| redis_cmd_line | Redis | The content of Redis command. | |
| rpc_time | xRPC | Time spent at the rpc request level. | |
| resp_body | core | This variable is only for logger and requires the logger to have a special configuration. | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which special configuration? I'm afraid that such a description is not useful for users to use this variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was update it

@mscb402 mscb402 requested a review from spacewander February 7, 2023 01:17
route_id = true,
route_name = true,
service_id = true,
service_name = true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the comma?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I readded it

@@ -48,5 +48,6 @@ APISIX 除了支持 [NGINX 变量](http://nginx.org/en/docs/varindex.html)外,
| service_name | core | APISIX 服务的名称。 | |
| redis_cmd_line | Redis | Redis 命令的内容。 | |
| rpc_time | xRPC | 在 RPC 请求级别所花费的时间。 | |
| resp_body | core | 此变量仅用于 Logger,并要求对应的 Logger 需要具备某些配置。 | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put it in alphabetical order

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -49,5 +49,6 @@ additional variables.
| service_name | core | Name of Service. | |
| redis_cmd_line | Redis | The content of Redis command. | |
| rpc_time | xRPC | Time spent at the rpc request level. | |
| resp_body | core | This variable is only for logger and requires the logger plugin to have a special configuration like `include_resp_body`. | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explain what this variable is, and what decides its value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

mscb402 and others added 2 commits February 9, 2023 09:17
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
soulbird
soulbird previously approved these changes Feb 9, 2023
@mscb402 mscb402 requested a review from tokers February 9, 2023 02:37
@spacewander spacewander merged commit 0d41d3a into apache:master Feb 10, 2023
hongbinhsu added a commit to fitphp/apix that referenced this pull request Feb 13, 2023
* upstream/master:
  docs: change the file name to 'create-ssl.py'.If 'ssl.py' is used as … (apache#8623)
  feat: Body transformer plugin (apache#8766)
  fix: mocking plugin panic when response_example contain $ (apache#8810) (apache#8816)
  feat: file logger plugin support response body in variable (apache#8711)
  docs(getting-started): rewrite the install section (apache#8807)
  feat: allow each logger to define custom log format in its conf (apache#8806)
  fix(etcd): reloaded data may be in res.body.node (apache#8736)
  fix: fix fetch all service info from consul (apache#8651)
  docs: fix global-rule.md wrong curl  address (apache#8793)
  feat: stream subsystem support consul service discovery (apache#8696)
  chore(kafka-logger): support configuration `meta_refresh_interval` parameter (apache#8762)
  feat: ready to release 2.15.2 (apache#8783)
@mscb402 mscb402 deleted the feat/file_logger_support_resp_body_var branch May 16, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants