identify: add metrics #2126
identify: add metrics #2126
Conversation
metrics added: address count protocol count peer push support
sukunrt
force-pushed
the
identify-metrics
branch
from
8341157
to
557aac3
Compare
dashboards/identify/identify.json
Outdated
| "title": "Outgoing Address Count", | ||
| "type": "gauge" |
There was a problem hiding this comment.
This looks a bit misleading since the gauge is never full. Any suggestions here?
There was a problem hiding this comment.
What about using a Stat? That way you'd also if a change brought number up or down:
dashboards/identify/identify.json
Outdated
| "title": "Outgoing Protocols Count", | ||
| "type": "gauge" | ||
| }, |
There was a problem hiding this comment.
Same here the gauge may be a bit empty and so a bit misleading.
p2p/protocol/identify/id.go
Outdated
| } | ||
| mes.ListenAddrs = append(mes.ListenAddrs, addr.Bytes()) | ||
| } | ||
| if ids.metricsTracer != nil { | ||
| ids.metricsTracer.NumProtocols(len(mes.Protocols)) | ||
| ids.metricsTracer.NumAddrs(len(mes.ListenAddrs)) | ||
| } |
There was a problem hiding this comment.
we can simplify this a bit if we are fine with counting snaphot.Addrs as Num Address Sent.
There was a problem hiding this comment.
I'm wondering if we should rework the API a bit. Might be nice to have a Identify{Push}Received(numProtos, numAddrs int). wdyt?
|
@marten-seemann left comments at a few places where I'm confused. Suggestions would be very helpful. |
p2p/protocol/identify/id.go
Outdated
| } | ||
| mes.ListenAddrs = append(mes.ListenAddrs, addr.Bytes()) | ||
| } | ||
| if ids.metricsTracer != nil { | ||
| ids.metricsTracer.NumProtocols(len(mes.Protocols)) | ||
| ids.metricsTracer.NumAddrs(len(mes.ListenAddrs)) | ||
| } |
There was a problem hiding this comment.
I'm wondering if we should rework the API a bit. Might be nice to have a Identify{Push}Received(numProtos, numAddrs int). wdyt?
sukunrt
force-pushed
the
identify-metrics
branch
from
07219a7
to
bc36042
Compare
sukunrt
force-pushed
the
identify-metrics
branch
from
bc36042
to
0f0778c
Compare
|
addressed review comments. This looks much cleaner now. I think we can remove counting peers by push support. On my node there are no peers with push not supported. So it doesn't look too useful. Do you think this metric would be useful? |
sukunrt
force-pushed
the
identify-metrics
branch
from
123f806
to
81f80d2
Compare
added metrics for:
address count
protocol count
peer push support