feat: dump interceptor #3118
feat: dump interceptor #3118
Conversation
lib/interceptor/dump.js
Outdated
| return true | ||
| } | ||
|
|
||
| // TODO: shall we forward the rest of the data to the handler or better to abort? |
There was a problem hiding this comment.
Suggestions? I might be had it wrong here 🤔
There was a problem hiding this comment.
you must keep the data flowing from the request or error/abort it. The latter will destroy the connection. It should be a user choice, but I would error/abort the socket by default.
Co-authored-by: Robert Nagy <[email protected]>
Do you mean to not directly abort the connection, but rather let it flow? Or how so? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3118 +/- ##
==========================================
- Coverage 94.17% 94.15% -0.03%
==========================================
Files 90 91 +1
Lines 24559 24692 +133
==========================================
+ Hits 23128 23248 +120
- Misses 1431 1444 +13 ☔ View full report in Codecov by Sentry. |
lib/interceptor/dump.js
Outdated
| return true | ||
| } | ||
|
|
||
| // TODO: shall we forward the rest of the data to the handler or better to abort? |
There was a problem hiding this comment.
you must keep the data flowing from the request or error/abort it. The latter will destroy the connection. It should be a user choice, but I would error/abort the socket by default.
|
@ronag ptal |
Co-authored-by: Robert Nagy <[email protected]>
lib/interceptor/dump.js
Outdated
| if (!this.#dumped) { | ||
| if (this.#aborted) { | ||
| this.#handler.onError(this.reason) | ||
| return | ||
| } | ||
|
|
||
| this.#handler.onError(err) | ||
| } |
There was a problem hiding this comment.
| if (!this.#dumped) { | |
| if (this.#aborted) { | |
| this.#handler.onError(this.reason) | |
| return | |
| } | |
| this.#handler.onError(err) | |
| } | |
| if (this.#dumped) { | |
| return | |
| } | |
| if (this.#aborted) { | |
| this.#handler.onError(this.reason) | |
| return | |
| } | |
| this.#handler.onError(err) |
lib/interceptor/dump.js
Outdated
| if (!this.#dumped) { | ||
| if (this.#aborted) { | ||
| this.#handler.onError(this.reason) | ||
| return | ||
| } | ||
|
|
||
| this.#handler.onComplete(trailers) | ||
| } | ||
| } |
There was a problem hiding this comment.
| if (!this.#dumped) { | |
| if (this.#aborted) { | |
| this.#handler.onError(this.reason) | |
| return | |
| } | |
| this.#handler.onComplete(trailers) | |
| } | |
| } | |
| if (this.#dumped) { | |
| return | |
| } | |
| if (this.#aborted) { | |
| this.#handler.onError(this.reason) | |
| } else { | |
| this.#handler.onComplete(trailers) | |
| } | |
| } |
lib/interceptor/dump.js
Outdated
| } | ||
|
|
||
| this.#maxSize = maxSize ?? this.#maxSize | ||
| this.#dumpOnAbort = dumpOnAbort ?? this.#dumpOnAbort |
There was a problem hiding this comment.
How about not having this as an option and just have it always enabled? We might also want to have a dump timeout in addition to a maxSize?
There was a problem hiding this comment.
I believe this goes in line with your suggestions for the PR.
Don't have strong opinion on this one, I believe there might be situations where you don't want to automatically dump on abort, but if we add it, it might never get used.
So let's simplify and follow your suggestions, if we see use cases or issues we can always add it 👍
There was a problem hiding this comment.
Regarding the timeout, shouldn't that already be covered by the bodyTimeout?
It might look redundant, and to have effect it might require to always set it lower to the bodyTimeout set for the Client/Agent
lib/interceptor/dump.js
Outdated
| if (this.#aborted) { | ||
| abort(this.#reason) | ||
| return | ||
| } |
There was a problem hiding this comment.
| if (this.#aborted) { | |
| abort(this.#reason) | |
| return | |
| } | |
| if (this.#aborted) { | |
| abort(this.#reason) | |
| return | |
| } |
This can't happen?
Co-authored-by: Robert Nagy <[email protected]>
Replied here 😅 |
Co-authored-by: Robert Nagy <[email protected]>
This relates to...
Rationale
Relates to #2835
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status