Add histogram support in metricstest #4177
Conversation
| @@ -55,29 +57,59 @@ type ( | |||
| sampleValue float64 | |||
| } | |||
|
|
|||
| histogramBucket struct { | |||
There was a problem hiding this comment.
open question: Should this be uppercased? So other packages can use this struct when running histogram tests?
There was a problem hiding this comment.
b/c lint complained, answer is yes
| if sample.metricType != dto.MetricType_HISTOGRAM { | ||
| return nil, fmt.Errorf("metric %s not a %s type", name, dto.MetricType_HISTOGRAM.String()) | ||
| } |
There was a problem hiding this comment.
I think we can get rid of this because this is the same as the condition we check before adding the sample
There was a problem hiding this comment.
...
gauge := handler.WithTags(gaugeTags...).Gauge(gaugeName)
gauge.Record(-2)
s2, err := handler.Snapshot()
require.NoError(t, err)
gaugeVal, err = s2.Counter(gaugeName+"_total", expectedGaugeTags...)
require.NoError(t, err)
I think these errors are mainly here to protect the test writer from making incorrect assumptions. For example for the test above, the test writer incorrectly thinks that a counter metric named: gaugeName+"_total" exists when its really a gauge.
There was a problem hiding this comment.
Ahh, I see. Makes sense to keep then
| if !maps.Equal(sample.labelValues, labelValues) { | ||
| return nil, fmt.Errorf("metric %s label mismatch, has %v, asked for %v", name, sample.labelValues, labelValues) |
There was a problem hiding this comment.
I think we should actually just ignore this sample instead of returning an error because that's how PromQL actually works. We should be operating on the samples whose labels match
| if !ok { | ||
| return nil, fmt.Errorf("metric %s not found", name) | ||
| } |
There was a problem hiding this comment.
I don't think this should be an error. I think we should return an empty list--that's how PromQL would behave
There was a problem hiding this comment.
These error checks are here to protect us/quickly signal any changes the underlying handler makes like adding more default labels or changing metric names.
https://github.com/temporalio/temporal/pull/3850/files#r1088378482
Here's one that this test case would've caught ^
There was a problem hiding this comment.
I see. Ok, yeah that makes sense
|
For the error-wrapping, we shouldn't be creating custom error classes, we should just do this: var (
ErrMetricNotFound = errors.New("metric not found")
Err...
)
func (s Snapshot) Histogram(name string, tags ...metrics.Tag) ([]HistogramBucket, error) {
...
if !ok {
return nil, fmt.Errorf("%w: %q", ErrMetricNotFound, name)
}
...
} |
* Add histogram support in metricstest * Address lint * Address comments * Address comments pt. 2 * Reorder fields * Wrap errors correctly * Use Err... naming convention for err vars
When you use the
metricstesthandler to record a histogram, you can now compare the resulting histogram bucket values.Unlike
counterorgaugesamples which return afloat64, forhistogramsamples, I introduced a new struct that contains a list ofhistogramBucket(s), that contain a value and upperBound.