-
Couldn't load subscription status.
- Fork 1.9k
Extend proxy #3105
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
Extend proxy #3105
Conversation
a79a62d to
92ec6fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ The following Jit checks failed to run:
- secret-detection-trufflehog
#jit_bypass_commit in this PR to bypass, Jit Admin privileges required.
More info in the Jit platform.
|
#jit_bypass_commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I've left some minor improvement suggestions.
| this.emit('data', connectionId, 'client->server', data); | ||
| serverSocket.write(data); | ||
|
|
||
| if(!this.interceptorInitializer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we can eliminate the branch prediction (if(!this.interceptorInitializer)) entirely by implementing a default "socket write/noop" interceptor that simply performs the socket write when no initializer is provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we can totally do this, in fact it was like that before, but i thought this is better. i will move it back
| interface ActiveConnection extends ConnectionInfo { | ||
| readonly clientSocket: net.Socket; | ||
| readonly serverSocket: net.Socket; | ||
| inflightRequestsCount: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about borrowing from node-redis's cache stats approach ? We could start by tracking inflightRequestsCount initially and add more metrics later. This design integrates well if you want to hook in traces, metrics, or logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only to distinguish between request/response and push for now. it will go away in nest iteration
The purpose for this PR is to extend the proxy to provide couple of new functionalities: