Add incoming/outgoing connections metrics #959
Conversation
metrics/metrics.go
Outdated
| @@ -38,6 +39,30 @@ const ( | |||
| ReshareShutdown ReshareStatus = "node_stopped" | |||
| ) | |||
|
|
|||
| type statsHandler struct { | |||
There was a problem hiding this comment.
this whole type is an IncomingConnectionsStatsHandler because it tags to the IncomingConnectionTimestamp metric.
Consider moving this statsHandler struct over to the net package, so that when e.g. the http client package pulls in metrics, it doesn't take on the grpc dependency.
There was a problem hiding this comment.
good call, thanks! Fixed.
mcamou
force-pushed
the
metrics
branch
3 times, most recently
from
368aa5f
to
268ebe6
Compare
|
@willscott @AnomalRoil I would appreciate some help regarding why |
net/client_grpc.go
Outdated
| @@ -311,9 +319,16 @@ func (g *grpcClient) conn(p Peer) (*grpc.ClientConn, error) { | |||
| metrics.GroupDialFailures.WithLabelValues(p.Address()).Inc() | |||
| } | |||
| } | |||
| g.conns[p.Address()] = c | |||
| metrics.GroupConnections.Set(float64(len(g.conns))) | |||
| if err != nil { | |||
There was a problem hiding this comment.
I don't understand where err is supposedly set now?
I think this is never run with the current code when ok == true
There was a problem hiding this comment.
You're right, and in the middle of solving this I figured out that we don't want to have the state as a label. I've fixed this and added a couple of comments explaining my reasoning.
There was a problem hiding this comment.
actually, the err = nil should not be necessary, but it's never a bad thing to be explicit:
gore version 0.5.3 :help for help
gore> var err error
gore> err
nil
There was a problem hiding this comment.
This line is unnecessary and doing the converse of what you want, no?
net/client_grpc.go
Outdated
| @@ -311,9 +319,16 @@ func (g *grpcClient) conn(p Peer) (*grpc.ClientConn, error) { | |||
| metrics.GroupDialFailures.WithLabelValues(p.Address()).Inc() | |||
| } | |||
| } | |||
| g.conns[p.Address()] = c | |||
| metrics.GroupConnections.Set(float64(len(g.conns))) | |||
| if err != nil { | |||
There was a problem hiding this comment.
This line is unnecessary and doing the converse of what you want, no?
net/client_grpc.go
Outdated
| } | ||
|
|
||
| // Emit the connection state regardless of whether it's a new or an existing connection | ||
| if err != nil { |
There was a problem hiding this comment.
I don't understand why you check if there was an error before firing the metric?
Why not fire it all the time?
There was a problem hiding this comment.
err will be non-nil only if grpc.Dial has an error, in which case c might be nil or unset. In that case, c.GetState() would bomb out.
There was a problem hiding this comment.
and yes, you're right above re: err != nil. Fixing both.
No description provided.