add support for logging streamed data in http_logger#497
Open
saninthottungal wants to merge 1 commit into
Open
add support for logging streamed data in http_logger#497saninthottungal wants to merge 1 commit into
saninthottungal wants to merge 1 commit into
Conversation
Contributor
Reviewer's GuideAdds support for logging streamed HTTP responses in TalkerHttpLogger by normalizing StreamedResponse into a regular Response for logging while returning an equivalent StreamedResponse to callers, and bumps the flutter_lints dev dependency in the example package. Sequence diagram for handling streamed HTTP responses in TalkerHttpLoggersequenceDiagram
participant Client
participant TalkerHttpLogger
participant HttpServer as HttpServerOrBackend
participant Talker
Client->>TalkerHttpLogger: interceptResponse(response)
TalkerHttpLogger->>HttpServer: response (may be StreamedResponse)
Note over TalkerHttpLogger: Check if response is StreamedResponse
alt response is StreamedResponse
TalkerHttpLogger->>TalkerHttpLogger: response.stream.toBytes()
TalkerHttpLogger->>TalkerHttpLogger: utf8.decode(bytes) -> body
TalkerHttpLogger->>TalkerHttpLogger: create Response(resForTalker)
TalkerHttpLogger->>TalkerHttpLogger: create StreamedResponse(resForReturn)
else response is Response
TalkerHttpLogger->>TalkerHttpLogger: resForTalker = response
TalkerHttpLogger->>TalkerHttpLogger: resForReturn = response
end
alt statusCode < 400 and settings.enabled
TalkerHttpLogger->>TalkerHttpLogger: settings.responseFilter(resForTalker)
opt responseFilter allows
TalkerHttpLogger->>Talker: logCustom(HttpResponseLog)
end
else settings.enabled
TalkerHttpLogger->>TalkerHttpLogger: settings.errorFilter(resForTalker)
opt errorFilter allows
TalkerHttpLogger->>Talker: logCustom(HttpErrorLog)
end
end
TalkerHttpLogger-->>Client: resForReturn
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- Reading the entire
StreamedResponseinto memory withtoBytes()before logging can be problematic for large responses; consider adding a size limit or truncation strategy to avoid excessive memory use while still capturing useful log information. - When decoding the streamed body with
utf8.decode(bytes), you may want to respect the response’sContent-Type/charset (or at least handle non-UTF-8 data more defensively) to avoid decoding errors or corrupt logs for binary or differently encoded responses. - The
StreamedResponse→Responseconversion logic could be extracted into a small helper to reduce duplication, make the intent clearer, and centralize future adjustments (e.g., handling additional fields or edge cases).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Reading the entire `StreamedResponse` into memory with `toBytes()` before logging can be problematic for large responses; consider adding a size limit or truncation strategy to avoid excessive memory use while still capturing useful log information.
- When decoding the streamed body with `utf8.decode(bytes)`, you may want to respect the response’s `Content-Type`/charset (or at least handle non-UTF-8 data more defensively) to avoid decoding errors or corrupt logs for binary or differently encoded responses.
- The `StreamedResponse` → `Response` conversion logic could be extracted into a small helper to reduce duplication, make the intent clearer, and centralize future adjustments (e.g., handling additional fields or edge cases).
## Individual Comments
### Comment 1
<location path="packages/talker_http_logger/lib/talker_http_logger_interceptor.dart" line_range="113" />
<code_context>
+
+ if (response is StreamedResponse) {
+ final bytes = await response.stream.toBytes();
+ final body = utf8.decode(bytes);
+ resForTalker = Response(
+ body,
</code_context>
<issue_to_address>
**issue:** Consider handling non-UTF8 / binary responses more defensively when decoding.
`utf8.decode(bytes)` will throw on non‑UTF8 bodies (e.g. binary or malformed payloads), causing the interceptor to fail instead of just logging. Consider using `utf8.decode(bytes, allowMalformed: true)` or wrapping the decode in try/catch and falling back to a safer representation (e.g. hex or `bytes.toString()`) when decoding fails.
</issue_to_address>
### Comment 2
<location path="packages/talker_http_logger/lib/talker_http_logger_interceptor.dart" line_range="112-121" />
<code_context>
+ BaseResponse resForReturn;
+
+ if (response is StreamedResponse) {
+ final bytes = await response.stream.toBytes();
+ final body = utf8.decode(bytes);
+ resForTalker = Response(
+ body,
+ response.statusCode,
+ headers: response.headers,
+ isRedirect: response.isRedirect,
+ persistentConnection: response.persistentConnection,
+ reasonPhrase: response.reasonPhrase,
+ request: response.request,
+ );
+
+ resForReturn = StreamedResponse(
+ Stream.value(bytes),
+ response.statusCode,
</code_context>
<issue_to_address>
**suggestion (performance):** Buffering the full streamed response body may be problematic for large payloads.
Reading the entire `StreamedResponse` into memory with `toBytes()` significantly increases memory usage and risks OOM for large or long-lived streams. Consider guarding this behavior (e.g., size limits, content-type checks, or a config flag to disable full-body logging for streamed responses) to avoid unbounded buffering.
Suggested implementation:
```
import 'dart:convert';
import 'package:talker/talker.dart';
import 'package:talker_http_logger/http_error_log.dart';
const int _kMaxLoggableStreamedResponseContentLength = 1024 * 1024; // 1 MiB
```
```
if (response is StreamedResponse) {
final contentLengthHeader = response.headers['content-length'];
final contentLength = response.contentLength ??
(contentLengthHeader != null ? int.tryParse(contentLengthHeader) : null);
final shouldBufferBody =
contentLength != null && contentLength <= _kMaxLoggableStreamedResponseContentLength;
if (shouldBufferBody) {
final bytes = await response.stream.toBytes();
final body = utf8.decode(bytes);
resForTalker = Response(
body,
response.statusCode,
headers: response.headers,
isRedirect: response.isRedirect,
persistentConnection: response.persistentConnection,
reasonPhrase: response.reasonPhrase,
request: response.request,
);
resForReturn = StreamedResponse(
Stream.value(bytes),
response.statusCode,
headers: response.headers,
isRedirect: response.isRedirect,
persistentConnection: response.persistentConnection,
reasonPhrase: response.reasonPhrase,
request: response.request,
);
} else {
// Avoid buffering large or unknown-size streamed responses to prevent high memory usage.
resForTalker = Response(
'',
response.statusCode,
headers: response.headers,
isRedirect: response.isRedirect,
persistentConnection: response.persistentConnection,
reasonPhrase: response.reasonPhrase,
request: response.request,
);
// Do not consume the stream; pass the original response through.
resForReturn = response;
}
```
To make this behavior configurable (as hinted in the review), you may also want to:
1. Add a configuration option to the interceptor to control `_kMaxLoggableStreamedResponseContentLength` (or to completely disable streamed-body logging).
2. Use that configuration instead of the hard-coded `1 MiB` constant so library consumers can tune or disable buffering based on their needs.
</issue_to_address>
### Comment 3
<location path="packages/talker_http_logger/lib/talker_http_logger_interceptor.dart" line_range="114-123" />
<code_context>
+ if (response is StreamedResponse) {
+ final bytes = await response.stream.toBytes();
+ final body = utf8.decode(bytes);
+ resForTalker = Response(
+ body,
+ response.statusCode,
+ headers: response.headers,
+ isRedirect: response.isRedirect,
+ persistentConnection: response.persistentConnection,
+ reasonPhrase: response.reasonPhrase,
+ request: response.request,
+ );
+
+ resForReturn = StreamedResponse(
+ Stream.value(bytes),
+ response.statusCode,
</code_context>
<issue_to_address>
**question (bug_risk):** Re-wrapping `StreamedResponse` as `Response` for logging/filters changes the response type surface.
For streamed responses, `resForTalker` is now a `Response` instead of `StreamedResponse`, so any `responseFilter`/`errorFilter` relying on `StreamedResponse` (type checks or streaming-specific behavior) will no longer work as before. If this change isn’t intentional, consider preserving the original type for filters/logging (e.g., log metadata plus a separately captured body), or clearly document that streamed responses will appear as `Response` in the logger path.
</issue_to_address>
### Comment 4
<location path="packages/talker_http_logger/lib/talker_http_logger_interceptor.dart" line_range="130-133" />
<code_context>
+ persistentConnection: response.persistentConnection,
+ reasonPhrase: response.reasonPhrase,
+ request: response.request,
+ contentLength: bytes.length,
+ );
+ } else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `bytes.length` as `contentLength` may diverge from the original response semantics.
The original `StreamedResponse` may have an explicit `contentLength` (including `-1` when unknown). Overwriting it with `bytes.length` can change behavior for consumers that rely on `contentLength` (e.g., progress, buffering). Prefer preserving `response.contentLength` when it’s non-null and non-negative, and only fall back to `bytes.length` when the original length is unknown.
```suggestion
reasonPhrase: response.reasonPhrase,
request: response.request,
contentLength: (response.contentLength != null &&
response.contentLength! >= 0)
? response.contentLength
: bytes.length,
);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| if (response is StreamedResponse) { | ||
| final bytes = await response.stream.toBytes(); | ||
| final body = utf8.decode(bytes); |
Contributor
There was a problem hiding this comment.
issue: Consider handling non-UTF8 / binary responses more defensively when decoding.
utf8.decode(bytes) will throw on non‑UTF8 bodies (e.g. binary or malformed payloads), causing the interceptor to fail instead of just logging. Consider using utf8.decode(bytes, allowMalformed: true) or wrapping the decode in try/catch and falling back to a safer representation (e.g. hex or bytes.toString()) when decoding fails.
Comment on lines
+112
to
+121
| final bytes = await response.stream.toBytes(); | ||
| final body = utf8.decode(bytes); | ||
| resForTalker = Response( | ||
| body, | ||
| response.statusCode, | ||
| headers: response.headers, | ||
| isRedirect: response.isRedirect, | ||
| persistentConnection: response.persistentConnection, | ||
| reasonPhrase: response.reasonPhrase, | ||
| request: response.request, |
Contributor
There was a problem hiding this comment.
suggestion (performance): Buffering the full streamed response body may be problematic for large payloads.
Reading the entire StreamedResponse into memory with toBytes() significantly increases memory usage and risks OOM for large or long-lived streams. Consider guarding this behavior (e.g., size limits, content-type checks, or a config flag to disable full-body logging for streamed responses) to avoid unbounded buffering.
Suggested implementation:
import 'dart:convert';
import 'package:talker/talker.dart';
import 'package:talker_http_logger/http_error_log.dart';
const int _kMaxLoggableStreamedResponseContentLength = 1024 * 1024; // 1 MiB
if (response is StreamedResponse) {
final contentLengthHeader = response.headers['content-length'];
final contentLength = response.contentLength ??
(contentLengthHeader != null ? int.tryParse(contentLengthHeader) : null);
final shouldBufferBody =
contentLength != null && contentLength <= _kMaxLoggableStreamedResponseContentLength;
if (shouldBufferBody) {
final bytes = await response.stream.toBytes();
final body = utf8.decode(bytes);
resForTalker = Response(
body,
response.statusCode,
headers: response.headers,
isRedirect: response.isRedirect,
persistentConnection: response.persistentConnection,
reasonPhrase: response.reasonPhrase,
request: response.request,
);
resForReturn = StreamedResponse(
Stream.value(bytes),
response.statusCode,
headers: response.headers,
isRedirect: response.isRedirect,
persistentConnection: response.persistentConnection,
reasonPhrase: response.reasonPhrase,
request: response.request,
);
} else {
// Avoid buffering large or unknown-size streamed responses to prevent high memory usage.
resForTalker = Response(
'',
response.statusCode,
headers: response.headers,
isRedirect: response.isRedirect,
persistentConnection: response.persistentConnection,
reasonPhrase: response.reasonPhrase,
request: response.request,
);
// Do not consume the stream; pass the original response through.
resForReturn = response;
}
To make this behavior configurable (as hinted in the review), you may also want to:
- Add a configuration option to the interceptor to control
_kMaxLoggableStreamedResponseContentLength(or to completely disable streamed-body logging). - Use that configuration instead of the hard-coded
1 MiBconstant so library consumers can tune or disable buffering based on their needs.
Comment on lines
+114
to
+123
| resForTalker = Response( | ||
| body, | ||
| response.statusCode, | ||
| headers: response.headers, | ||
| isRedirect: response.isRedirect, | ||
| persistentConnection: response.persistentConnection, | ||
| reasonPhrase: response.reasonPhrase, | ||
| request: response.request, | ||
| ); | ||
|
|
Contributor
There was a problem hiding this comment.
question (bug_risk): Re-wrapping StreamedResponse as Response for logging/filters changes the response type surface.
For streamed responses, resForTalker is now a Response instead of StreamedResponse, so any responseFilter/errorFilter relying on StreamedResponse (type checks or streaming-specific behavior) will no longer work as before. If this change isn’t intentional, consider preserving the original type for filters/logging (e.g., log metadata plus a separately captured body), or clearly document that streamed responses will appear as Response in the logger path.
Comment on lines
+130
to
+133
| reasonPhrase: response.reasonPhrase, | ||
| request: response.request, | ||
| contentLength: bytes.length, | ||
| ); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Using bytes.length as contentLength may diverge from the original response semantics.
The original StreamedResponse may have an explicit contentLength (including -1 when unknown). Overwriting it with bytes.length can change behavior for consumers that rely on contentLength (e.g., progress, buffering). Prefer preserving response.contentLength when it’s non-null and non-negative, and only fall back to bytes.length when the original length is unknown.
Suggested change
| reasonPhrase: response.reasonPhrase, | |
| request: response.request, | |
| contentLength: bytes.length, | |
| ); | |
| reasonPhrase: response.reasonPhrase, | |
| request: response.request, | |
| contentLength: (response.contentLength != null && | |
| response.contentLength! >= 0) | |
| ? response.contentLength | |
| : bytes.length, | |
| ); |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #497 +/- ##
==========================================
- Coverage 98.63% 92.20% -6.43%
==========================================
Files 3 7 +4
Lines 146 231 +85
==========================================
+ Hits 144 213 +69
- Misses 2 18 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of all, this may not be the best way to implement this, i created a PR instead of creating this as an issue because you may get better context of what the feature is about. that being said, i am open to make any changes to this PR.
Thanks for the amazing SDK
Summary by Sourcery
Add support for logging HTTP streamed responses while preserving the original response stream behavior.
New Features:
Enhancements:
Build: