[Storage][Logging]Let users opt in logging_request_body - #19710
Conversation
| if isinstance(http_request.body, types.GeneratorType): | ||
| _LOGGER.debug("File upload") | ||
| else: | ||
| if self.logging_request_body or options.pop("logging_request_body", False): |
There was a problem hiding this comment.
I didn't touch on_response because logging response body is handled pretty well.
For download blob, since we are doing streaming download, no actually body will be logged. For other responses logging response body is helpful, since the response body could contains raw error message or some xml elements we want to check.
Let me know if you think we should disable logging for any kinds of response body .
There was a problem hiding this comment.
We should make it symetric and possibly consider single switch for both request and response like .net and java do.
.net https://github.com/Azure/azure-sdk-for-net/search?p=1&q=IsLoggingContentEnabled&type=
java https://github.com/Azure/azure-sdk-for-java/blob/3f31d68eed6fbe11516ca3afe3955c8840a6e974/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLogDetailLevel.java
There was a problem hiding this comment.
besides some may want to disable body logging due to PII being present inside.
| # We don't want to log the binary data of a file upload. | ||
| _LOGGER.debug("Hidden body, please use logging_body to show body") |
There was a problem hiding this comment.
I'd rather delete this else branch. It's going to produce lot of entries that are not very useful.
Instead consider adding logger.debug in ctor/ctors if logging is enabled but body logging is not. so that there's some hint about that option but is not rendered on each request.
There was a problem hiding this comment.
I'll leave this like this for now, and put it in the TODO so it will be picked up later.
| _LOGGER.debug("File attachments: %s", filename) | ||
| elif response.http_response.headers.get("content-type", "").endswith("octet-stream"): | ||
| elif resp_content_type.endswith("octet-stream"): | ||
| _LOGGER.debug("Body contains binary data.") | ||
| elif response.http_response.headers.get("content-type", "").startswith("image"): | ||
| elif resp_content_type.startswith("image"): | ||
| _LOGGER.debug("Body contains image data.") |
There was a problem hiding this comment.
we could consider dumbing binary payloads in encoded (base64 for example) form. (not blocking this PR).
|
Is there a reason we've stalled on this? Xiaoxi Fu (@xiafu-msft) Kamil Sobol (@kasobol-msft) |
commented
Oct 15, 2021
just I forgot it |
Resolves #19419