MQE: Add support for changes function

[lamida]

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

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

[jhesketh]

Nice work, thanks for that!

I think it's also worth putting this function through TestCompareVariousMixedMetricsVectorSelectors to test some edge cases (just add it to line 2310).

[charleskorn]

[nit] We use the term "Prometheus' engine" to refer to it - "PromQL engine" is ambiguous because MQE is a PromQL engine too.

Suggested change

	// PromQL engine doesn't support histogram for `changes` function yet,
	// Prometheus' engine doesn't support histogram for `changes` function yet,

Might also be good to add a link to the Prometheus' issue tracking adding support for native histograms in changes, if there is one.

[lamida]

I tried to find but it seems there are no issue tracking for that.

[jhesketh]

Charles is likely referring to this one: prometheus/prometheus#13934

[lamida]

That issue didn't mention changes. This is the comment that mentioned the need to add support (code link).

[lamida]

@jhesketh @charleskorn I have addressed all the comments. These are changes from the previous reviews:

• Ring-buffer wrap around is now checked.
• Moved the function logic from range_vector.go to fpoint_ring_buffer.go so that I can test ring-buffer wrap around easier. Let me know if anyone have concern.

[jhesketh]

I think this is better fit into functions/range_vectors in terms of consistency. Can we not create a test in the functions package and pass it a constructed types.RangeVectorStepData?

[lamida]

I think this is better fit into functions/range_vectors in terms of consistency

Actually some functions are implemented in fpoint_ring_buffer.go too like CountAtOrBefore, LastAtOrBefore and AnyAtOrBefore.

Can we not create a test in the functions package and pass it a constructed types.RangeVectorStepData?
It is not possible to create the buffer to simulate wrap around because points and other fields are private. I prefer not to make them public just for testing.

Other than that, if we put the logic in range_vectors.go we are exposing the internal representation of the ring buffer in range_vector.go. Like on how iterating head and tail which I believe should be internal fpoint_ring_buffer.go.

At the end I don't have any strong opinion and I am fine if there is a better argument.

[lamida]

Can we not create a test in the functions package and pass it a constructed types.RangeVectorStepData?

It is not possible to create the buffer to simulate wrap around because the fields are private.

[charleskorn]

I would favour this not being on the ring buffer type - ChangesAtOrBefore can efficiently be implemented by using UnsafePoints, whereas CountAtOrBefore, LastAtOrBefore and AnyAtOrBefore don't need the slices produced by UnsafePoints and so benefit from being implemented directly on the ring buffer type.

[lamida]

I will moved back to range_vectors.go then. I will figure out how the best to test that.

[jhesketh]

This is consistent with how we use accumulate elsewhere. I don't think there is much overhead in range when the len is 0 vs using an if.

Suggested change

	// The points buffer is wrapped around, therefore we need to check changes starting from the buffer's head
	// and then continue to the tail.
	if len(fHead) > 0 && len(fTail) > 0 {
	// We don't need to compare the first point with itself, hence start from fHead[1:]
	accumulate(fHead[1:])
	accumulate(fTail)
	}
	if len(fHead) > 0 && len(fTail) == 0 {
	// We don't need to compare the first point with itself, hence start from fHead[1:]
	accumulate(fHead[1:])
	}
	// The points buffer is wrapped around, therefore we need to check changes starting from the buffer's head
	// and then continue to the tail.
	// We don't need to compare the first point with itself, hence start from fHead[1:]
	accumulate(fHead[1:])
	accumulate(fTail)

[lamida]

You are right. We can simplify that. Updated in a7d0474

[jhesketh]

Would be good to have a test where the value doesn't change at the wrap around

[lamida]

Added in af5c867

Did not mean to apporve yet

[charleskorn]

LGTM modulo suggestion below

[jhesketh]

lgtm too, but agree with Charles. Thanks Jon!