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

7595 - Be able to add the body's content as field with http_response #7596

Merged
merged 1 commit into from
Jun 8, 2020
Merged

7595 - Be able to add the body's content as field with http_response #7596

merged 1 commit into from
Jun 8, 2020

Conversation

essobedo
Copy link
Contributor

@essobedo essobedo commented May 28, 2020

closes #7595

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@essobedo essobedo changed the title 7595 - Be able to add the body content as field with http_response 7595 - Be able to add the body's content as field with http_response May 28, 2020
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

I think we need to include a few checks before we can do this safely.

Make sure the body is valid utf-8. While technically a string field could contain any data, we want to have all strings from Telegraf to be valid utf-8. If the body is not valid utf-8, log an error and report body_read_error.

Add the ability to set a limit on the body size: response_body_max_size = "1KiB". Check the influxdb_listener for an example using http.MaxBytesReader. If it is exceeded report a body_read_error and log.

Let's also call the new option: response_body_field = "body". Default will be "" which means don't add.

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 1, 2020
@essobedo
Copy link
Contributor Author

essobedo commented Jun 2, 2020

@danielnelson thx for your feedback. I did the corresponding changes however please note that regarding response_body_max_size:

  1. I made it non optional with a default value of 32 Mo (it is big but we did not have any limit before this change)
  2. I decided not to use MaxBytesReader because it seems to be for request body only such that you end up with an error message of type "http: request body too large" (as you can see here) which would be misleading I believe so I used io.LimitReader instead and made it read one byte above the limit to know if the limit has been reached.

WDYT of these changes?

@essobedo
Copy link
Contributor Author

essobedo commented Jun 2, 2020

@danielnelson please note that the CI fails because of another test that doesn't seem to be related to those changes

@danielnelson danielnelson added this to the 1.15.0 milestone Jun 8, 2020
@danielnelson danielnelson merged commit bf0f674 into influxdata:master Jun 8, 2020
@essobedo essobedo deleted the 7595/add_body_to_reponse branch June 9, 2020 06:58
@essobedo
Copy link
Contributor Author

essobedo commented Jun 9, 2020

@danielnelson thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Be able to add the body's content as field with http_response
2 participants