feat: support histograms for command latency statistics #2721
Conversation
rabunkosar-dd
changed the title
rabunkosar-dd
changed the title
rabunkosar-dd
changed the title
|
@rabunkosar-dd Thanks for your contribution. You use |
Co-authored-by: Twice <[email protected]>
|
@rabunkosar-dd Could you please fix the lint issue? |
| @@ -43,6 +45,13 @@ enum StatsMetricFlags { | |||
|
|
|||
| constexpr int STATS_METRIC_SAMPLES = 16; // Number of samples per metric | |||
|
|
|||
| // Experimental part to support histograms for cmd statistics | |||
| struct CommandHistogram { | |||
| std::vector<std::shared_ptr<std::atomic<uint64_t>>> buckets; | |||
There was a problem hiding this comment.
It seems the atomic numbers inside the vector are not shared. Is it necessary to use shared_ptr here?
There was a problem hiding this comment.
I used that as a workaround, since vector will not support atomic directly.
There was a problem hiding this comment.
There are basically two things:
since vector will not support atomic directly.
That's not totally correct. You can use std::vector<std::atomic>, but then some methods of vector will lead to compilation errors due to that atomic doesn't have copy ctor and copy assign op, e.g. push_back. Hence an obvious method is to avoid push_back, since we already know the size of the vector (i.e. make the vector a fixed-size one), then it can pass the compilation, with vector<atomic> being used.
I used that as a workaround
If you want to wrap it inside a smart pointer, I recommend std::unique_ptr.
|
This PR adds an option to build kvrocks with histogram support to breakdown the command latencies. Current statistics are based on the total latency and the command count, giving averages. For our use case, where tail latencies are very impactful, this was not sufficient to track the general performance of the instances.
It also adds a config parameter to define the bucket boundaries. I will also follow up with a PR to add similar support in kvrocks_exporter (and support additive histograms for prometheus).
The buckets are not additive and there is an implicit bucket that tracks values beyond the defined values (inf bucket).
I added a build paramater (ENABLE_HISTOGRAMS), when set to ON will compile the support into the binary.The feature is enabled if the config parameter defines a non-empty list of buckets that are coma separated