Add a new hook for before and after a connection happens

[pfreixes]

The idea is extending the current Hook interface [1] for adding two new available functions, for example BeforeConnect and AfterConnect, that would be called before a new connection is asked and before.

These two new hooks could be used for extending observability that the client side, providing a reliable metric on the cost of creating new connections in terms of latency.

Connection errors could be also provided as a parameter of the AfterConnect function.

I could take care of making the proper PR with the implementation.

[1] https://github.com/go-redis/redis/blob/6e4eb2e3acad9575fb1e2c8690a3c1decf2e59e5/redis.go#L24

[vmihailenco]

Sounds good to me 👍

I believe this new hook should be optional:

type ConnectEvent struct {
    Err error
}

type ConnectHook interface {
    BeforeConnect(ctx context.Context) context.Context
    AfterConnect(ctx context.Context, event ConnectEvent)
}

type fullHook interface {
    Hook
    ConnectHook
}

type hooks struct {
	hooks []fullHook
}

Then in Redis.AddHook you can check if the provided hook implements the fullHook interface and upgrade it to fullHook if necessary (just use a wrapper with empty *Connect methods).

Does that make sense?

[vmihailenco]

What do you think about using a Dialer option instead:

redis.NewClient(&redis.Options{
    Dialer: func(ctx context.Context, network, addr string) (net.Conn, error) {
        fmt.Println("before connect")
        conn, err := net.Dial(network, addr)
        fmt.Println("after connect")
        return conn, err
    }
})

[pfreixes]

Not really fan, but if we can not find a proper way for adding the hooks could be an alternative.

What would be you red flags or no go in terms of design? breaking the current contract of hooks? Ill would like to keep stick to the hooks pattern.

For example what about adding the BeforeConnect and AfterConnect as first citizen hooks, none optionals. Requesting to the user to update the hooks interface and providing noop functions if the user is not interested on tracing the connection.

[vmihailenco]

This is how we could keep the existing Hook interface as is and add ConnectHook - #1791

But now I think that we should recommend using Options.Dialer and not add such a hook. Not many people need such functionality so cumbersome API should not be a problem.

[pfreixes]

Oks, seems that this is pretty much done, do you need any help?

[pfreixes]

Sorry for the late response here, Ive been working on a delta version of your proposal which moves all of the pool hook objects under the pool module, but still using as entrypoint for adding a hook what you proposed. Here a draft on how this could look like pfreixes@1a01c44

For the sake of calling the hooks if and only if a new connection is created, which is responsibility of the Pool, and for avoid doing the wiring at Get method level, just decided that maybe would be worth it to add the hooks at Pool type level, and then leave to it the responsibility to call it when its necessary.

[monkey92t]

It sounds good, but I can hardly imagine what benefits this hook can bring? Why should we add it?
If you just observe the cost of creating a connection, I think it’s easier to use Options.Dialer and Options.OnConnect.

[pfreixes]

Measuring latency is an extension in terms of observability to the current stats [1]. Connections can be created at any moment under many different circumstances, pool creation, spike of traffic, connection recycling due to errors (timeouts), ....

Latency of the connection creation is not rare in the industry

[1] https://github.com/go-redis/redis/blob/master/internal/pool/pool.go#L36

[pfreixes]

just for the sake of curiosity

Some people may not use the default net.Dial method but customize the connection function

Any chance to have some examples of the kind of customisation that people use to make by providing alternative dial functions?

[pfreixes]

No hard feelings for me, but a proposal here [1] in case you are still interested on adding the hooks instead of using the override of the dial command.

Its still missing a solution for the cloned clients since current implementation won't create a new slice of connection pool hooks for each cloned client, any recommendation will be more than welcomed.

If there is no a good solution but you are still believe that this solution is cleaner and could get eventually merged we might think on moving the connection pool hooks under the Redis type, with all of the consequences that it would have (having to provide the hooks as a parameter for the pool.Get method)

[1] #1803

[vmihailenco]

Latency of the connection creation is not rare in the industry

That was my impression as well when we created tracing instrumentation for OpenTelemetry. We used to measure dial, read, and write timings.

But in practice it was not very useful. Dials happen so rarely that you can't even draw a nice chart for it. Reads/writes happen regularly, but all the time is spent waiting/reading data from sockets so in the end it is just (expensive to process) noise.

So I think that we don't need a separate hook given that you can wrap Dialer to get the timings. And we are going to add proper metrics to go-redis using OpenTelemetry Metrics (in 1-2 months). Connection-related metrics will be added first. I think that should cover 99% needs and the rest can be achieved with (clumsy) dialer-wrapping API.

[MakingL]

I now want to test the latency of my connection establishment. I will initiate multiple connections concurrently to test the connection processing capability of my server, so I need to look at all connection establishment latency. Can you give some suggestions to implement this function through Dialer? @vmihailenco