-
Notifications
You must be signed in to change notification settings - Fork 2
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.
Thank you @aarshkshah1992, some initial feedback
ccc7141
to
3565ec6
Compare
This will make things easier when we have to add cars.
This gives us more useful buckets (same width)
It is a good practice to avoid defining buckets with values that may change. By hardcoding bucket definitions this way, we avoid breaking grafana boards when someone decides to adjust a timeout. If we ever need to change buckets, or add new one with bigger max, but keep old ones intact, we can define them by hand.
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.
Thank you @aarshkshah1992!
I've pushed cosmetic changes:
- small renames to make it clear which metrics are about blocks. this is in preparation for future CAR/graph fetches (Support non-block requests #45) that will be done in addition to block ones
- linear buckets for size (we will most likely see majority being ~256 KiB and ~1 MiB blocks on heatmap)
- some precautions to not break grafana boards using histograms (see ℹ️ below)
// Size buckets from 256 KiB (default chunk in Kubo) to 4MiB (maxBlockSize), 256 KiB wide each | ||
blockSizeHistogram = prometheus.LinearBuckets(262144, 262144, 16) |
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.
ℹ️ This gives us more useful buckets (same width), and we can plot them in Grafana using heatmap widget
// TODO: Speed max bucket could use some further refinement, | ||
// for now we don't expect speed being bigger than transfering 4MiB (max block) in 500ms | ||
speedHistogram = prometheus.ExponentialBucketsRange(1, 4194304/500, 20) | ||
|
||
// Duration max bucket is informed by the timeouts per block and per peer request/retry | ||
durationPerBlockHistogram = prometheus.ExponentialBucketsRange(1, 60000, 10) | ||
durationPerBlockPerPeerHistogram = prometheus.ExponentialBucketsRange(1, 20000, 10) |
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.
ℹ️ I've just remembered that it is a good practice to avoid defining buckets with values that may
change. By hardcoding bucket definitions this way, we avoid breaking grafana boards when someone decides to adjust a timeout.
If we ever need to change buckets, or add new one with bigger max, but keep old ones intact, we can define them as explicit list of values (like we did with legacy ones in kubo a while ago):
[]float64{0.05, 0.1, 0.25, 0.5, 1, 2, 5, 10, 30, 60}
fixes #34