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(kafka-logger): add max req/resp body size attributes #11133

Merged

Conversation

shreemaan-abhishek
Copy link
Contributor

Description

In cases where the req/response body is very large and when include_req_body/resp_body is enabled this leads to very high CPU usage and can even cause unavailability. To fix such event, two new attributes are introduced:

  • max_resp_body_bytes
  • max_req_body_bytes

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)

@shreemaan-abhishek shreemaan-abhishek changed the title feat(kfk-logger): add max req/resp body size attributes feat(kafka-logger): add max req/resp body size attributes Apr 9, 2024
@shreemaan-abhishek shreemaan-abhishek marked this pull request as ready for review April 10, 2024 01:10
@@ -22,6 +23,7 @@ local bp_manager_mod = require("apisix.utils.batch-processor-manager")
local math = math
local pairs = pairs
local type = type
local req_body = ngx.req.read_body
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
local req_body = ngx.req.read_body
local req_read_body = ngx.req.read_body

ctx._body_buffer[ctx._plugin_name] = nil
return body_buffer
if max_resp_body_bytes and #body_data >= max_resp_body_bytes then
body_data = str_sub(body_data, 1, max_resp_body_bytes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to sub the body_data again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in 213, we just sub the chunk this happens in the case when this is the first and last chunk of body_filter phase. (since eof == true and body_buffer is nil).

in 221 we need to sub because it is the last chunk but not the very first one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if it's the first chunk, it will be dealed in line ~202

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm now it makes sense but I think we are calling sub on 213 on chunk because body_buffer is nil. And on 221 we are calling it on body data formed after concat_tab the body_buffer table.

@moonming moonming merged commit ea69104 into apache:master Apr 16, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants