Add metrics reporting #37
Conversation
|
We may want to make this PR against the stabilize branch. |
|
@yusefnapora the slice definitely plays nicely with our metrics package. you can always iterate the values and build the slice yourself. it's equivalent to a |
table.go
Outdated
| @@ -69,23 +73,44 @@ func (rt *RoutingTable) Update(p peer.ID) (evicted peer.ID, err error) { | |||
| bucketID = len(rt.Buckets) - 1 | |||
| } | |||
|
|
|||
| var full = 0 | |||
There was a problem hiding this comment.
something as intensive as this should maybe be enabled with a flag. could just be a module scope var
metrics/metrics.go
Outdated
| recordWithBucketIndex(ctx, bucketIndex, KBucketPeersRemoved.M(1)) | ||
| } | ||
|
|
||
| var DefaultViews = map[string]*view.View{ |
There was a problem hiding this comment.
Initialisation of views should be separate from the construction of the default views. Also DefaultViews should be a slice so that when registered it looks like, err := view.Register(kbmetrics.DefaultsViews...).
metrics/metrics.go
Outdated
| // aren't sufficient. However, they should be updated using the functions below to avoid | ||
| // leaking OpenCensus cruft throughout the rest of the code. | ||
| var ( | ||
| KBucketsFull = stats.Int64(MeasureBucketsFull, |
There was a problem hiding this comment.
nitpick: If you are multi-lining a function call, put each parameter on a new line and not a combination. In this particular case, the third parameter is lost at the end of the line.
table.go
Outdated
| func (rt *RoutingTable) Update(p peer.ID) (evicted peer.ID, err error) { | ||
| // UpdateAndRecordMetrics adds or moves the given peer to the front of its respective bucket, while recording | ||
| // metrics about bucket capacities and peer additions and removals. | ||
| func (rt *RoutingTable) UpdateAndRecordMetrics(ctx context.Context, p peer.ID) (evicted peer.ID, err error) { |
There was a problem hiding this comment.
Recording metrics shouldn't be considered 'extra' functionality, it is just a requirement of running a production system, if the rename is because of the change to the parameters, it is a common pattern to append Ctx to the function name when the only change is to add the ctx parameter to the function signature, i.e. UpdateCtx.
There was a problem hiding this comment.
Thanks, I didn't like the new name and am glad there's an idiom I can use 😄
metrics/metrics.go
Outdated
| // indicating the index of the bucket to which the measurement applies. | ||
| func recordWithBucketIndex(ctx context.Context, bucketIndex int, ms ...stats.Measurement) { | ||
| _ = stats.RecordWithTags(ctx, | ||
| []tag.Mutator{tag.Upsert(keyBucketIndex, string(bucketIndex))}, |
There was a problem hiding this comment.
tag.Upsert is a performance killer, unfortunately, so the way that this helper operates results in a lot of extra allocations. I suggest creating a helper that does the same as LocalContext but for bucket index and gets called after the bucketID is defined, just once.
table.go
Outdated
| @@ -69,23 +73,44 @@ func (rt *RoutingTable) Update(p peer.ID) (evicted peer.ID, err error) { | |||
| bucketID = len(rt.Buckets) - 1 | |||
| } | |||
|
|
|||
| var full = 0 | |||
| var nonEmpty = 0 | |||
| for i, buck := range rt.Buckets { | |||
There was a problem hiding this comment.
Ideally, we could get a snapshot of all the buckets straight away, but it is really expensive. I think a better way to approach this would be to build up the view of all buckets one bucket at a time. So whichever bucket is chosen for the Update operation is the one we record metrics for.
There was a problem hiding this comment.
Good idea - the measures for the # of full and non-empty buckets are redundant with the utilization measure anyway. I'll rewrite this to just record utilization for the buckets we actually visit in the Update method and remove the loop.
- renames the Update and Remove methods that accept a Context to UpdateCtx and RemoveCtx. - removes the full and non-empty bucket measures (since they can be derived from utilization. - removes the iteration of all buckets on update and only records utilization for the bucket being modified. - exports default views individually and in a slice
yusefnapora
force-pushed
the
feat/bucket-metrics
branch
from
189b505
to
487d482
Compare
defining the metrics ctx just before use looks nicer
So, last week I was really bothered about libp2p/go-libp2p-kad-dht#283, but after I got over my stomach bug and got my optimism back, I realized that this is a great opportunity to flex our new testlab and measure the impact of requiring peers to be connected.
To get us towards a meaningful test scenario, this adds some metrics about k-bucket utilization. It's a bit awkward because the
Updatemethod where pretty much all the metrics are recorded doesn't have aContextparameter, so I made it default tocontext.Backgroundand added a newUpdateAndRecordMetricsoption that takes aContext.Also not sure if I like how the measures and views are exported - I came across the discussion at libp2p/go-libp2p-kad-dht#327 and agree that long-term we need to have a standard way to do this. For now I stuffed the views in a map keyed by measure name, but that was before I realized that Go doesn't have a
Map.Valuesmethod, so now I kind of hate it and will probably just put them in a slice after all.Anyway, hopefully this is close-ish to what we want - all feedback welcome.