Measure bloom filter AppGossip hit rate #4085
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds instrumentation to measure the effectiveness of the bloom filter used in the gossip pull mechanism.
- Track and compute bloom filter hits and misses in
AppRequest - Introduce a
bloomFilterHitRatehistogram in the metrics and register it - Extend unit tests to cover the new histogram and the
computeBloomFilterHitPercentagefunction
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| network/p2p/gossip/handler.go | Count filter hits/misses and compute hit percentage |
| network/p2p/gossip/gossip.go | Add bloomFilterHitRate histogram and register it |
| network/p2p/gossip/gossip_test.go | Update tests to assert metric observation and new logic |
Comments suppressed due to low confidence (3)
network/p2p/gossip/gossip.go:115
- [nitpick] Metric name
bloomfilter_hit_rateis inconsistent with the project’s snake_case convention; consider renaming tobloom_filter_hit_rateor prefixing withgossip_to match other metric names.
Name: "bloomfilter_hit_rate",
network/p2p/gossip/gossip_test.go:633
- [nitpick] Consider adding tests for the error branches in
computeBloomFilterHitPercentage(e.g., whensafemath.AddorMuloverflows) to verify thatokis false in those cases.
func TestComputeBloomFilterHitPercentage(t *testing.T) {
network/p2p/gossip/handler.go:18
- The computeBloomFilterHitPercentage function uses zap.Uint64 and zap.Error but
go.uber.org/zapis not imported, causing a compile error. Addimport "go.uber.org/zap"or switch to the existing logging API.
safemath "github.com/ava-labs/avalanchego/utils/math"
This commit adds a metric to measure the bloom filter hit rate % for the gossip pull queries. Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
network/p2p/gossip/handler.go
Outdated
| total, err := safemath.Add(hits, misses) | ||
| if err != nil { | ||
| log.Warn("failed to calculate total hits and misses", | ||
| zap.Uint64("hits", hits), | ||
| zap.Uint64("misses", misses), | ||
| zap.Error(err), | ||
| ) | ||
| return 0, false | ||
| } | ||
|
|
||
| hitsOneHundred, err := safemath.Mul(hits, 100) | ||
| if err != nil { | ||
| log.Warn("failed to calculate hit ratio", | ||
| zap.Uint64("hits", hits), | ||
| zap.Uint64("misses", misses), | ||
| zap.Error(err), | ||
| ) | ||
| return 0, false | ||
| } |
There was a problem hiding this comment.
I wonder if we can get rid of these overflow checks... since the bloom filter is []byte (which can be at most of size max int) could we just emit a metric on both the hits + length of the filter? We could figure out the hit/miss % in dashboards with metric math.
There was a problem hiding this comment.
I wonder if we can get rid of these overflow checks... since the bloom filter is []byte (which can be at most of size max int) could we just emit a metric on both the hits + length of the filter? We could figure out the hit/miss % in dashboards with metric math.
I am not sure what you mean by the length of the filter. The bloom filter is constant size and its length is always the same, but yet independent of the elements that it can "contain".
I guess you meant the size of the mempool? Since the maximum number of hits we can get is the size of the mempool, not the bloom filter. Sure I can do that, that's a good point.
There was a problem hiding this comment.
Unfortunately we don't have a Size() on the set interface so I will try to code it without the safemath.
There was a problem hiding this comment.
Changed the code to not use safemath.
|
|
||
| testHistogram := &testHistogram{ | ||
| Histogram: metrics.bloomFilterHitRate, | ||
| } | ||
| metrics.bloomFilterHitRate = testHistogram | ||
|
|
There was a problem hiding this comment.
This looks a bit hacky to me, if we want to do an assert that the metrics are the values we're expecting would it make sense for us to consider using prometheus/testutil here?
There was a problem hiding this comment.
testutil.ToFloat64 doesn't work with histograms. Do you have a suggestion?
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Why this should be merged
It can be insightful to measure how effective and efficient is our gossip pull mechanism that is based on bloom filters.
How this works
This commit adds a metric to measure the bloom filter hit rate % for the gossip pull queries.
How this was tested
TestGossiperGossipaccordingly to reflect the metricsNeed to be documented in RELEASES.md?
No.