libp2p / go-libp2p

libp2p implementation in Go
MIT License
5.83k stars 1.04k forks source link

proposal: expose transport stats #1729

Open marten-seemann opened 1 year ago

marten-seemann commented 1 year ago

Motivating issue: Get accurate RTT measurement for yamux (https://github.com/libp2p/go-yamux/pull/70).

We could introduce an transport.StatTransport (suggestions for a better name appreciated!) interface that transports can (but don't need to) implement:

type StatTransport {
     Stat() Stat
}

type Stat struct {
     RTT time.Duration
     RTTVariance time.Duration
}

Both TCP and QUIC should be able to expose these values easily. We already have an implementation for TCP: https://github.com/libp2p/go-libp2p/blob/fcf408c65d99f9a2f45c761a1003d66d3a664f30/p2p/transport/tcp/metrics.go#L240-L249 quic-go could expose these on the quic.Connection, the congestion controller is already keeping track of these values.

Open question: In the future, we'll probably want to add more values to Stat. Obvious candidates are congestion window, packet loss rate, counters for bytes sent and received, etc. Will every transport be able to determine these values?

cc @vyzo @Stebalien @MarcoPolo @BigLep

Stebalien commented 1 year ago

:+1:

Will every transport be able to determine these values?

I'd just ensure that we have reasonable zero values. We may also want to break this into something like:

type RTTStat struct {
    RTT time.Duration
}
type Stat struct {
    RTT *RTTStat
}
marten-seemann commented 1 year ago

How would we set the congestion window then? Like this?

type Stat struct {
     CongestionWindow *uint64
}

Alternatively, we could say that for values where the zero value doesn't make sense, we don't need to use a pointer. That would apply to RTT, RTT variance and cwnd.

Stebalien commented 1 year ago

Alternatively, we could say that for values where the zero value doesn't make sense, we don't need to use a pointer. That would apply to RTT, RTT variance and cwnd.

Yeah, I'd personally go with that approach where possible.

marten-seemann commented 1 year ago

Just noticed, of course this doesn't live at the transport level, but at the connection level. This means we can either extend the ConnStats struct: https://github.com/libp2p/go-libp2p/blob/fcf408c65d99f9a2f45c761a1003d66d3a664f30/core/network/network.go#L100-L104

or introduce a new struct for that.