MQE: Add support for histogram_quantile #9929
Conversation
jhesketh
requested review from
stevesg,
grafanabot and
a team
as code owners
Also preps support for more classic histogram functions to come. (Will require some re-work, but the basics are there). Tidies up annotation tests and checks their results between engines. (Since sometimes we emit annotations with results, and sometimes the results are omitted when there is an annotation).
jhesketh
force-pushed
the
jhesketh/mqe-histogram-quantile
branch
from
38d1a77
to
d293f3a
Compare
jhesketh
force-pushed
the
jhesketh/mqe-histogram-quantile
branch
from
d89bbab
to
78fc811
Compare
jhesketh
force-pushed
the
jhesketh/mqe-histogram-quantile
branch
from
78fc811
to
5abeee0
Compare
| @@ -295,7 +295,7 @@ func TestOurTestCases(t *testing.T) { | |||
| prometheusEngine := promql.NewEngine(opts.CommonOpts) | |||
|
|
|||
| testdataFS := os.DirFS("./testdata") | |||
| testFiles, err := fs.Glob(testdataFS, "ours*/*.test") | |||
There was a problem hiding this comment.
Why has the asterisk been removed? We want to run tests from both the ours and ours-only directories.
| ` | ||
| // TODO add series with malformed le? |
There was a problem hiding this comment.
Is this TODO still valid?
| // Because we test a huge amount of combinations, if we get a failure, stop running the other tests. | ||
| // This will reduce the output to make it easier to manage. | ||
| shouldStop := false | ||
|
|
There was a problem hiding this comment.
This functionality is built into go test: use go test -failfast.
| } | ||
|
|
||
| func getMixedMetricsForTests() ([]string, int, string) { | ||
| func getMixedMetricsForTests(includeClassicHistograms bool) ([]string, int, string) { |
There was a problem hiding this comment.
Looks like includeClassicHistograms is always true - is this needed?
| results = append(results, res) | ||
|
|
||
| _, infos := res.Warnings.AsStrings(testCase.expr, 0, 0) | ||
| //require.ElementsMatch(t, testCase.expectedWarningAnnotations, warnings) |
There was a problem hiding this comment.
Should this be commented out?
| "bad bucket label warning": { | ||
| data: mixedClassicHistograms, | ||
| expr: `histogram_quantile(0.5, series{host="c"})`, | ||
| expectedWarningAnnotations: []string{"PromQL warning: bucket label \"le\" is missing or has a malformed value of \"abc\" for metric name \"series\" (1:25)"}, |
There was a problem hiding this comment.
[nit] Might be clearer to use raw strings here to avoid the need for all the escaping
| @@ -1836,11 +1836,82 @@ func (t *timeoutTestingQueryTracker) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func TestAnnotations(t *testing.T) { | |||
There was a problem hiding this comment.
Is there a reason why you've split this test in half?
| // If both results are available, compare them (sometimes we skip prometheus) | ||
| if len(results) == 2 { | ||
| // We do this extra comparison to ensure that we don't skip a series that may be outputted during a warning | ||
| // or vice-versa where no result may be expected etc. | ||
| testutils.RequireEqualResults(t, testCase.expr, results[0], results[1]) | ||
| } |
There was a problem hiding this comment.
Do we need to compare the results when we're testing annotations? I'd expect the results to be exercised elsewhere.
There was a problem hiding this comment.
Could you please add a test for the case where the buckets for an output series change over time (eg. at T=1, buckets are 1, 2 and 5, but at T=2, buckets are 1, 3 and 7).
Also preps support for more classic histogram functions to come. (Will require some re-work, but the basics are there).
Tidies up annotation tests and checks their results between engines. (Since sometimes we emit annotations with results, and sometimes the results are omitted when there is an annotation).
What this PR does
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.