-
Notifications
You must be signed in to change notification settings - Fork 841
Measure bloom filter AppGossip hit rate #4085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
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.