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 |
bigs
left a comment
bigs
left a comment
There was a problem hiding this comment.
generally looks pretty good. probably good to target the stabilize branch as steven mentioned. let's test this out w/ prometheus and grafana!
table.go
Outdated
| 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
lanzafame
left a comment
lanzafame
left a comment
There was a problem hiding this comment.
Looks good, mainly just adjustments to reduce the perf overhead, which in general, equates to reducing the number of tags added and not recording any stats in loops.
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
|
|
||
| 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
lanzafame
left a comment
lanzafame
left a comment
There was a problem hiding this comment.
Would love for the multiline nitpick to be addressed, but am happy to see this merged either way.
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.