Proxy refactorings #3433
Proxy refactorings #3433
Conversation
hlinnaka
requested review from
nikitakalyanov,
funbringer,
SomeoneToIgnore and
a team
and removed request for
a team and
nikitakalyanov
hlinnaka
force-pushed
the
proxy-refactorings
branch
from
eba6250 to
95fd68d
Compare
| impl<S, W> MeasuredStream<S, W> { | ||
| pub fn new(stream: S, inc_write_count: W) -> Self { | ||
| impl<S> MeasuredStream<S> { | ||
| pub fn new(stream: S, write_counter: prometheus::IntCounter) -> Self { |
There was a problem hiding this comment.
Sorry, this is where we'll draw a line.
For sure, there are good changes in this PR, but I don't like this change, because it's not a refactoring, it's a bikeshed. One of the core benefits of being a project maintainer is the ability to choose and maintain a certain style. What's the point of being a lead if you can't even code the way you enjoy?
There was a problem hiding this comment.
Ok, so the reason I wanted to simplify the signature of MeasuredStream is that with the other refactorings in this PR, the generic becomes quite cumbersome.
To demonstrate the problem, here's a branch with the MeasuredStream change reverted: https://github.com/neondatabase/neon/tree/heikki/proxy-refactorings-without-measuredstream-simplification. It doesn't compile. I couldn't figure out how to fill in the generic type parameters to make it compile. I'm sure there is a way, but to me the simplest solution seemed to be to remove the unused abstraction. What solution would you prefer?
It was generalized so that you could pass a custom function that is called whenever data is flushed. The only use case we have for it was to increment a prometheus counter, so let's dismantle the abstraction, and just pass a prometheus IntCounter to it. Simplifies the code a little bit. If we need the abstraction again in the future, we can always add it back.
This also splits the 'connect_to_db' function, so that it only establishes the connection, and a new 'handle_connection' function is the equivalent of what 'connect_to_db' used to do. This made it easier to attach a span to specifically to the first part where we establish the connection.
It's not a property of the credentials that we receive from the client, so remove it from ClientCredentials. Instead, pass it as an argument directly to 'authenticate' function, where it's actually used. All the rest of the changes is just plumbing to pass it through the call stack to 'authenticate'
The name "Client" was a bit ambiguous. Instead of encapsulating all the data needed to establish the connection, change it so that it encapsulates the streams, after the connection has been established. With that, "EstablishedConnection" is a fitting name for it.
There was a lot of duplicated code. The resulting shared function now uses two tracing spans, one for establishing the connections, and a separate span for forwarding the traffic after that. This makes for nicer traces in the future, because you can dig into how long the startup phase takes, and where the time is spent.
hlinnaka
force-pushed
the
proxy-refactorings
branch
from
95fd68d to
6fcf0f2
Compare
|
As discussed moving to draft |
SomeoneToIgnore
requested review from
koivunej
and removed request for
SomeoneToIgnore
A bunch of refactorings in proxy. I started doing these while I was trying to add suitable tracing spans to the authentication code. I wanted to structure code so that we have one span for the start of a connection, where we perform authentication and establish the "client<->proxy" and "proxy<->database" connections, and a separate span after the connection is fully established and we are just forwarding the bytes. When you combine that with the traces from control plane and compute, you get a nice distributed trace that shows where exactly the time is spent when a client connects.
This PR doesn't add the tracing yet, but after these changes, it'll be much more straightforward to add.