Stats for TCP dials

[davestibrany-dd]

Hi Rueian,

We're interested in getting stats on TCP dials, such as the number of successful/erroneous dials, and traces to help determine dial latency.

The easiest way I can think to implement this would be to have an ClientOption.DialFn and call it from dial, if it exists. What do you think?

Also, we'd be happy to contribute this.

[rueian]

Hi @davestibrany-dd, thank you for your interest in contributing to this.

I think it is a good idea and a ClientOption.DialFn of func(string, *net.Dailer, *tls.Config) (net.Conn, error) would be good.

[rueian]

Hi @proost,

The ClientOption.DialFn has already been implemented. Do you mean you want to contribute built-in TCP dials stats? If so, I think it can be implemented under the rueidisotel package by leveraging the ClientOption.DialFn.

[proost]

Yes, I mean built-in TCP dial stats.

I plan to use prometheus for stats, not open telemetry. is it okay to locate in rueidisotel?

[rueian]

OpenTelemery Metrics can be output in prometheus format. We don't need to import the github.com/prometheus/client_golang module.

[proost]

you're right. OpenTelemetry Metrics can be output in prometheus format.

My plan is before & after dialing instrument some metrics. So it is hard to do in rueidisotel package because can't inject hooks or wrapping. Above case, just lettting user do their own instrumentation logic in ClientOption.DialFn is better than providing built-in metrics.

[rueian]

Indeed, with the current rueidisotel.WithClient, it is hard to do hooks or wrapping. But I think we can add a new rueidisotel.NewClient. We already have something similar to that:

rueidis/rueidisaside/aside.go

Line 28 in 3851dec

func NewClient(option ClientOption) (cc CacheAsideClient, err error) {

.

[proost]

@rueian OK, then i briefly show what i plan to do.

I made a new constructor like rueidisaside package in the rueidisotel package. In the new constructor, i wrap ClientOption.DialFn.

I will record 7 metrics

1. how much attempt to dial
2. how much success to dial
3. how much fail to dial
4. how many connections exist
5. how many connections are closed
6. how much failed to close connection
7. how much longer to dial
   and wrapping functions are looks like below:

type trackedConn struct {
    net.Conn
    connCloseFailure metric.Int64Counter
    totalConns metric.Int64UpDownCounter
}

func (c *trackedConn) Close() error {
     err := c.Conn.close()
     if err != nil {
          // Add 1 to connection close failure counter
         return err
    }

    // decrease 1 to total connection updown counter
    return nil
}

func reportDialing(dst string, dialer *net.Dialer, tlsConfig *tls.Config) (net.Conn, err) {
    start := time.now()
   
    // Add 1 to dialing attempt counter

    conn, err := clientOptions.DialFn(dst, dialer, tlsConfig) // call original dialfn in client option
    if err != nil {
         // Add 1 to dialing failure counter
         return nil, err
    }
    
    // Add dialing latency observable counter
    // Add 1 to dialing success counter
    // Add 1 to total connection updown counter

    return &trackedConn{
          Conn: conn,
          connCloseFailure:  client.connCloseFailure,
          totalConns: client.totalConns,
    }, nil
}

If you think it is useless metrics or add/delete more metrics give me a feedback.

[rueian]

Hi @proost,

The proposal looks good generally. However, I think some metrics can be removed:

1. how much attempt to dial
2. how much success to dial
3. how much fail to dial This can be reflected from the 1 and 2.
4. how many connections exist
5. how many connections are closed This can be reflected from the 2 and 4.
6. how much failed to close connection I believe we don't need to consider this since there is nothing we can do if a Close() fails.
7. how much longer to dial <- This is tricky. If we provide it, we must also make its histogram buckets configurable.

There is one more important thing regarding the wrapping net.Conn: Its Close() must tolerate multiple calls to it and only count once.

[proost]

how much longer to dial <- This is tricky. If we provide it, we must also make its histogram buckets configurable.

If i'm correct, opentelemtry use views to change histogram bucket. If i get MeterProvider as constructor option, then it can be solved simply.

[rueian]

Hi @proost,

Do you have examples or references to the views you mentioned? I am not aware of that.

[proost]

Oh, sorry i misunderstood. you're right. configured bucket option must be given.

[rueian]

Completed: #421