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

Changed HttpLoggingPolicy to ensure a response is logged even if its body is never consumed by a client. #39964

Merged
merged 7 commits into from
May 1, 2024

Conversation

vcolin7
Copy link
Member

@vcolin7 vcolin7 commented Apr 30, 2024

Currently, the HttpLoggingPolicy.LoggingHttpResponse class we use for logging a response will only log when either its Flux<ByteBuffer> getBody() or BinaryData getBodyAsBinaryData() methods get called (I assume to avoid eagerly buffering the response body). This causes a situation where policies such as BearerTokenAuthenticationPolicy don't log the response because they don't care about accessing the body but only the headers.

We're changing the logic in HttpLoggingPolicy to log the response no matter if the body is accessed or not. If the HttpLogLevel does not include the body (e.g. HEADERS), then it will not be consumed eagerly, otherwise we will include it in the log. We assume body logging is used in non-production scenarios, as it is more resource intensive, and it usually is enabled only when debugging issues.

As for tests, there are still a few failing ones, so I'll spend some time this week fixing that scenario.

@vcolin7 vcolin7 added Client This issue points to a problem in the data-plane of the library. Azure.Core azure-core labels Apr 30, 2024
@vcolin7 vcolin7 self-assigned this Apr 30, 2024
# Conflicts:
#	sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java
Copy link
Member

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

We should ensure we close/consume bodies whenever possible because, orthogonal to logging, this signals underlying HTTP client implementation that response is consumed and it can return the underlying connection to the pool where it can be used for other requests.

Not closing the response body leads (orthogonally to logging) to connection staying 'busy' waiting for someone to consume the response, creating a bottleneck for the application.

While I have no objections eagerly buffering when request/response body logging is enabled, I believe we need to solve a bigger problem (if) we leak connections.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@vcolin7 vcolin7 merged commit 7391dc1 into main May 1, 2024
20 checks passed
@vcolin7 vcolin7 deleted the feature/core/eager-body-logging branch May 1, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core azure-core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants