Histogram percentile support
Opened this issue · 7 comments
Hey,
are there plans to give PCPHistorgram a percentile support in a way that when update function is called, it provides the instances? One idea would be to have an array of percentiles user is interested in and those would be added as instances named "perc_99" or "perc_95". For simplicity, just integer percentiles would be fine (50, 90, 95, 99). Would you accept such a patch?
If this is not planned or wanted, what is the best way of "plugging-in" the update function so percentiles gets passed into PCP? Maybe a callback function or similar pattern would do it so I could write my own handler.
Thanks!
@lzap the Histogram embeds pcpInstanceMetric (https://sourcegraph.com/github.com/performancecopilot/speed/-/blob/metrics.go#L1373:3), which makes it able to get the properties of an Instance Metric, but the pcpInstanceMetric type is internal to the library, the PCPInstanceMetric type is external and also implements the InstanceMetric interface.
The current instances added to PCP from the histogram are at (https://sourcegraph.com/github.com/performancecopilot/speed/-/blob/metrics.go#L1429:79). So to add custom instances for PCPHistogram, a simple way is to simply change the embedding from pcpInstanceMetric to PCPInstanceMetric, that way you'd get SetInstance and ValInstance from InstanceMetric (https://sourcegraph.com/github.com/performancecopilot/speed/-/blob/metrics.go#L427:6)
Would you accept such a patch?
Of course, I'm open to patches around any usecases that I've been unable to forsee.
Would you prefer:
- exporting instance so it can be modified externally
- ability to provide array of percentiles via constructor
- hardcoding 50, 90, 95 and 99 percentiles into source code
@lzap my original idea for this was to simply export ValInstance and SetInstance from *PCPHistogram (via PCPInstanceMetric). But that seems too generic for this. I think providing the percentiles to track as instances in the constructor would be a great option.
I have also been thinking of doing functional options for all constructors in the library for some time, so you could do
NewPCPHistogram(
...
WithPercentiles(50, 90, 95, 99),
...
)but this needs to be done for all types, and we'll probably pick this up this year in GSoC. Thoughts, @natoscott ?
@suyash will talk to our student. His primary focus is on implementing metadata label support, which we should also consider using here. We can label histogram metrics as such, as well as individual instances as being percentiles - across all of PCP - allowing generic clients / monitoring tools to be able to consume histogram metrics meaningfully.
@natoscott the idea is largely based on https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis. This needs to be done only for constructor functions (New...), basically providing functional arguments for optional parameters. It would be a good opportunity to deep dive into the library, and a good initial metric for evaluation.