Add double_exponential_smoothing MQE function #10873

lamida · 2025-03-13T09:51:52Z

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

pkg/streamingpromql/engine_test.go

pkg/streamingpromql/testdata/ours/functions.test

pkg/streamingpromql/operators/functions/range_vectors.go

pkg/streamingpromql/testdata/ours/functions.test

pkg/streamingpromql/functions.go

pkg/streamingpromql/engine_test.go

pkg/streamingpromql/operators/functions/range_vectors_test.go

pkg/streamingpromql/operators/functions/range_vectors.go

pkg/streamingpromql/engine_test.go

pkg/streamingpromql/functions.go

pkg/streamingpromql/operators/functions/range_vectors_test.go

pkg/streamingpromql/operators/functions/range_vectors.go

pkg/streamingpromql/testdata/ours/functions.test

pkg/streamingpromql/engine_test.go

pkg/streamingpromql/functions.go

lamida · 2025-03-14T09:27:06Z

@charleskorn @jhesketh thanks for the review. The implementation is following this wikipedia formula but also I try to follow the Prometheus upstream implementation.

Several things to note are the following points:

When we initialize b0 which depends on where are samples between head or tail of the buffer instead of just doing b0=x1-x0. But this is pretty straightforward.
Most comments and concerns are around this point: When we start calculate bt. In prometheus as I mentioned in some comments above, bt is calculated only from 3rd sample onward.

Seeing the test are passed for both upstream and "ours" test, I am pretty sure the implementation is already correct. Please suggest some test data that can prove this wrong.

I also inline the calcTrend call to make it look clearer here: 1a82d9a

lamida · 2025-03-14T09:36:52Z

@jhesketh replying #10873 (comment) (not sure why I can't reply inline there.

Can you also please add a test for when there are only native histograms?
There won't be any annotation emitted if samples are only native histogram.

The operator will just return.

	if (len(fHead) + len(fTail)) < 2 {
		return 0, false, nil, nil
	}

So I don't think there will be any annotation we can test there. We can test for no annotation emitted but I believe it will be pointless.

Please let me know if I understand you incorrectly.

charleskorn · 2025-03-16T22:05:10Z

So I don't think there will be any annotation we can test there. We can test for no annotation emitted but I believe it will be pointless.

I think it'd be worthwhile adding a test for this: this codifies that we expect that no annotation is returned when only native histograms are selected.

pkg/streamingpromql/operators/functions/range_vectors.go

jhesketh · 2025-03-16T23:24:41Z

@jhesketh replying #10873 (comment) (not sure why I can't reply inline there.

Can you also please add a test for when there are only native histograms?
There won't be any annotation emitted if samples are only native histogram.

The operator will just return.
	if (len(fHead) + len(fTail)) < 2 {
		return 0, false, nil, nil
	}
So I don't think there will be any annotation we can test there. We can test for no annotation emitted but I believe it will be pointless.

Please let me know if I understand you incorrectly.

Yes, that was my intention. That we check we have the same annotations, or lack-thereof, as Prometheus. This can be one as a .test file.

lamida · 2025-03-18T07:05:57Z

@jhesketh added test 8826a7f

pkg/streamingpromql/testdata/ours/functions.test

pkg/streamingpromql/engine_test.go

pkg/streamingpromql/operators/functions/range_vectors.go

pkg/streamingpromql/operators/functions/range_vectors_test.go

pkg/streamingpromql/testutils/utils.go

pkg/streamingpromql/operators/functions/range_vectors.go

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

…smoothing Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

jhesketh · 2025-03-27T04:28:31Z

pkg/streamingpromql/engine_test.go

 	// Test each label individually to catch edge cases in with single series
 	labelCombinations := testutils.Combinations(labelsToUse, 1)
+
+	// We tried to have this test with 2 labels, but it was failing due to the inconsistent ordering of prometheus processing matchers that result in multiples series, e.g series{label=~"(c|e)"}.
+	// Prometheus might process series c first or e first which will trigger different validation errors for second and third parameter of double_exponential_smoothing.


I think it's helpful to explain why it gives a different validation error as you explained to me earlier.
It's not a different argument to the function, it's a different value in the range vector.
That is because the range vector of the series being computed against, values are skipped for the native histograms until it gets to a value where it has a float. That aligns with a different scalar value for the argument and thus gives a different error.

Effectively it is the different argument that is passed. But there is a part of how native histogram being skipped too. I added your point here da3a0a3.

It's not clear to me why, if we might get different errors, we skip comparing the values - shouldn't we skip comparing the errors?

When we get validation error, Value will be nil and we got panic. We still want to compare the error to ensure the validation is correct. More detail discussion is in this comment: https://github.com/grafana/mimir/pull/10873/files/b1eb614926072c1a01437ddd2e6f53b176459960..aaebaa28df8302ecae7669f0ee65be795718831e#r2015549294

If result.Err is not nil, then shouldn't result.Value always be nil?

If so, then I think we can drop skipValueComparison and modify RequireEqualResults to ensure that both results have the same error (or both don't have an error), and if both have the same error, then both Values should be nil.

I am still thinking the original check to just return and skip Value comparison if they are nil is the simplest.

But let me attempt to still address this suggestion.

to ensure that both results have the same error (or both don't have an error)

We can achieve just using the existing first check

require.Equal(t, expected.Err, actual.Err)

and if both have the same error, then both Values should be nil.

if expected.Err != nil && actual.Err != nil && expected.Err.Error() == actual.Err.Error() { require.Nil(t, expected.Value) require.Nil(t, actual.Value) return }

The proposed change is already applied in the last commit.

Note: I can't use errors.Is(expected.Err, actual.Err). It won't check the error as the same although the error string is same.

pkg/streamingpromql/testutils/utils.go

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

lamida · 2025-03-27T04:55:59Z

@jhesketh

I think it would be more useful to have a flag to RequireEqualResults for skipValueComparisonOnError, so we only skip it if we've also gotten an error and then pass that through from the relevant test (if that makes sense?)

A bit large changes, but this should address your feedback: 611e1b1

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

charleskorn · 2025-03-27T05:11:00Z

pkg/streamingpromql/operators/functions/range_vectors_test.go

+		args         args
+		wantFloat    float64
+		wantHasFloat bool
+		wantErr      bool


wantErr is never used - should we just remove it?

Removed 18188b0

charleskorn · 2025-03-27T05:11:31Z

pkg/streamingpromql/operators/functions/range_vectors_test.go

+)
+
+func Test_runDoubleExponentialSmoothing(t *testing.T) {
+	type args struct {


[nit] Why not just inline these fields into the struct below?

Make it little clearer which one is arguments and which one are wants. I think I will just keep it this way.

charleskorn · 2025-03-27T05:15:42Z

pkg/streamingpromql/engine_test.go

 	// Test each label individually to catch edge cases in with single series
 	labelCombinations := testutils.Combinations(labelsToUse, 1)
+
+	// We tried to have this test with 2 labels, but it was failing due to the inconsistent ordering of prometheus processing matchers that result in multiples series, e.g series{label=~"(c|e)"}.
+	// Prometheus might process series c first or e first which will trigger different validation errors for second and third parameter of double_exponential_smoothing.


It's not clear to me why, if we might get different errors, we skip comparing the values - shouldn't we skip comparing the errors?

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

pkg/streamingpromql/operators/functions/range_vectors_test.go

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

This reverts commit 611e1b1. Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

charleskorn

LGTM modulo suggestion below

charleskorn · 2025-03-27T09:19:13Z

pkg/streamingpromql/testutils/utils.go

@@ -20,6 +20,13 @@ import (
 // It's possible that floating point values are slightly different due to imprecision, but require.Equal doesn't allow us to set an allowable difference.
 func RequireEqualResults(t testing.TB, expr string, expected, actual *promql.Result, skipAnnotationComparison bool) {
 	require.Equal(t, expected.Err, actual.Err)
+
+	if expected.Err != nil && actual.Err != nil && expected.Err.Error() == actual.Err.Error() {


This should be sufficient:

Suggested change

if expected.Err != nil && actual.Err != nil && expected.Err.Error() == actual.Err.Error() {

if expected.Err != nil {

We've already checked that expected.Err and actual.Err are equal above, so we only need to check if there's an error.

lamida mentioned this pull request Mar 13, 2025

Mimir Query Engine #10067

Open

lamida marked this pull request as ready for review March 13, 2025 18:35

lamida requested a review from a team as a code owner March 13, 2025 18:35

charleskorn reviewed Mar 14, 2025

View reviewed changes

lamida requested a review from charleskorn March 14, 2025 03:09

lamida enabled auto-merge (squash) March 14, 2025 03:13

charleskorn reviewed Mar 14, 2025

View reviewed changes

pkg/streamingpromql/engine_test.go Outdated Show resolved Hide resolved

pkg/streamingpromql/operators/functions/range_vectors_test.go Outdated Show resolved Hide resolved

pkg/streamingpromql/operators/functions/range_vectors.go Outdated Show resolved Hide resolved

jhesketh reviewed Mar 14, 2025

View reviewed changes

charleskorn reviewed Mar 14, 2025

View reviewed changes

pkg/streamingpromql/functions.go Outdated Show resolved Hide resolved

charleskorn reviewed Mar 16, 2025

View reviewed changes

pkg/streamingpromql/operators/functions/range_vectors.go Outdated Show resolved Hide resolved

lamida requested review from jhesketh and charleskorn March 18, 2025 07:51

jhesketh reviewed Mar 21, 2025

View reviewed changes

pkg/streamingpromql/testdata/ours/functions.test Show resolved Hide resolved

charleskorn mentioned this pull request Mar 26, 2025

MQE: remove unneeded feature toggles #11002

Merged

1 task

charleskorn reviewed Mar 26, 2025

View reviewed changes

lamida added 10 commits March 26, 2025 16:15

Add double_exponential_smoothing skeleleton

d7ba9bc

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

First attemp implement double_exponential_smoothing

8ce527e

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Enable tests

73fbea1

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Move scalar validation in the step function

b736eea

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Remove double_exponential_smoothing from unsupported function list

fa7aee7

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Narrow down experimental function testing to only double_exponential_…

ed9198a

…smoothing Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Fix incorrect scalar index

e9fa71e

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Simplify the closure

202e18d

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Handle series dedup correctly

8bc4b54

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add our test to test circular buffer boundary

ecf5e34

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

lamida added 5 commits March 27, 2025 11:31

Remove test that give inconsistent result from prometheus

f8ed0fd

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add comment on why we don't have dynamic sf and tf test

b3b72bb

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Remove unnecessary anymore return from testutil

d0de323

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Use boolean to track when to start computing tf

19ad21c

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Fix typo

f21cb91

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

lamida requested review from charleskorn and jhesketh March 27, 2025 03:53

lamida added 2 commits March 27, 2025 12:17

Flip the flag to match variable name

b1eb614

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add dynamic tf sf test with only one label test

aaebaa2

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

jhesketh reviewed Mar 27, 2025

View reviewed changes

lamida added 3 commits March 27, 2025 12:37

Update comment

da3a0a3

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Another typo

2af58ca

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add skipValueComprison in RequreEqualResults

611e1b1

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Fix lint

aed289e

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

charleskorn reviewed Mar 27, 2025

View reviewed changes

Remove unused want

18188b0

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

charleskorn reviewed Mar 27, 2025

View reviewed changes

pkg/streamingpromql/operators/functions/range_vectors_test.go Outdated Show resolved Hide resolved

lamida added 6 commits March 27, 2025 14:14

Check no error

da4ab46

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add forgotten value type check if branch

165ceda

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Revert "Add skipValueComprison in RequreEqualResults"

b3132f7

This reverts commit 611e1b1. Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Try to address feedback

b1f86fc

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add more checks

d5788ed

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

Add one last fix on the test suggestion

bbe04a6

Signed-off-by: Jon Kartago Lamida <me@lamida.net>

lamida requested review from jhesketh and charleskorn March 27, 2025 07:13

charleskorn approved these changes Mar 27, 2025

View reviewed changes

lamida merged commit 187f406 into main Mar 27, 2025
26 checks passed

lamida deleted the lamida/mqe-double-exponential-smoothing branch March 27, 2025 09:19

lamida mentioned this pull request Mar 27, 2025

Address last nits from double_exponential_smoothing PR #11023

Merged

4 tasks

	if expected.Err != nil && actual.Err != nil && expected.Err.Error() == actual.Err.Error() {
	if expected.Err != nil {

Add double_exponential_smoothing MQE function #10873

Add double_exponential_smoothing MQE function #10873

Uh oh!

Conversation

lamida commented Mar 13, 2025

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lamida commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lamida commented Mar 14, 2025

Uh oh!

charleskorn commented Mar 16, 2025

Uh oh!

Uh oh!

jhesketh commented Mar 16, 2025

Uh oh!

lamida commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lamida Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lamida commented Mar 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charleskorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lamida commented Mar 14, 2025 •

edited

Loading

lamida Mar 27, 2025 •

edited

Loading