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

Adding Request Body to RequestLogger #401

Merged
merged 2 commits into from Jul 24, 2015
Merged

Conversation

jarjee
Copy link
Contributor

@jarjee jarjee commented Jul 23, 2015

The body is correctly retrieved, but is never included in the final log message. This commit adds a request body field to the log output.

While the body is correctly retrieved, at no point was it actually
included in the logs.
@andrewthad
Copy link
Contributor

Quick question. If the request body is binary data (like an image), would this potentially send control characters to the terminal and mess up the logs? You might want to do something like call decodeUtf8' on it first to make sure that it's textual data and not binary data.

@snoyberg
Copy link
Member

Agreed with @andrewthad. Also, the current behavior is intended for just capturing form fields, which are in fact being logged. Logging the full request body is a new piece of functionality. We should also take into account the fact that the request body may be very large, and therefore only a subset should likely be displayed.

To prevent garbage data from polluting the logs, we check to see if the
data can be safely be turned into utf8. If not, we ignore the line.
@jarjee
Copy link
Contributor Author

jarjee commented Jul 23, 2015

@andrewthad Just tested it, and it can send control characters to the terminal. Whoops. I've made a commit to use decodeUtf8'.
@snoyberg The current default is to only display a body as long as it is <2048, which may still be too large. I'd imagine that this is something that would probably be best if it is tweak-able, since this part of the logger is specifically for debugging output, after all.

snoyberg added a commit that referenced this pull request Jul 24, 2015
Adding Request Body to RequestLogger
@snoyberg snoyberg merged commit ba7715e into yesodweb:master Jul 24, 2015
@snoyberg
Copy link
Member

Thanks!

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.

None yet

3 participants