query-frontend: Ensure queries are properly closed #11157

jhesketh · 2025-04-09T06:17:49Z

Make sure for each execution of a query we properly call the Close() interface. This is necessary where an engine (such as MQE) pools result slices.

What this PR does

Which issue(s) this PR fixes or relates to

Fixes: #5713

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.

Make sure for each execution of a query we properly call the Close() interface. This is necessary where an engine (such as MQE) pools result slices. Fixes: grafana#5713

jhesketh · 2025-04-09T12:29:20Z

pkg/frontend/querymiddleware/spin_off_subqueries_queryable.go

+		// FIXME: newSeriesSetFromEmbeddedQueriesResults copies samples, but does not do a deep copy of histogram spans.
+		// We either need to do a deep copy of the histogram spans or somehow wrap the SeriesSets with a closer.
+		// Labels also need closer inspection as they appear to be pointers too.


I'm inclined to suggest we do a deep copy in these cases. I would have to investigate the impact, but the complexity of changing the Queryable interface or doing something else seems quite high.

That coupled with the possibility that any change here may be short lived after the introduction of the planner.

CC @charleskorn @julienduchesne

That coupled with the possibility that any change here may be short lived after the introduction of the planner.

I think this is the deciding factor for me, and the fact that histograms aren't yet widely used - so I'm OK with the idea of doing a deep copy of histograms.

Labels also need closer inspection as they appear to be pointers too.

Can you elaborate on this?

Can you elaborate on this?

Labels are not copied, they are also pointers:

mimir/pkg/frontend/querymiddleware/sharded_queryable.go

Line 322 in 67a7b0e

set = append(set, series.NewConcreteSeries(mimirpb.FromLabelAdaptersToLabels(stream.Labels), samples, histograms))

mimir/pkg/mimirpb/compat_slice.go

Line 22 in 67a7b0e

func FromLabelAdaptersToLabels(ls []LabelAdapter) labels.Labels {

Although likely not an issue for MQE at the moment, if we start pooling label resources it may become one. Thus we likely need to consider deep copying labels as well at this point.

labels.Labels instances are not pointers:

with stringlabels (current default), they are a string

without stringlabels (old default), they are an immutable slice of labels

mimir/pkg/mimirpb/compat_slice.go

Line 22 in 67a7b0e

func FromLabelAdaptersToLabels(ls []LabelAdapter) labels.Labels {

is only used if stringlabels is not being used, and stringlabels has been the default for some time (might be worth proposing removing it to avoid confusion).

mimir/pkg/mimirpb/compat_stringlabels.go

Line 13 in 67a7b0e

func FromLabelAdaptersToLabels(ls []LabelAdapter) labels.Labels {

is the equivalent for stringlabels.

labels.Labels instances are not pointers

No, but if MQE starts to pool them and re-use them we need to be aware of when we Close the query. My point was more that we might need to put some protection or be careful here, not that they are currently an issue.

We can't pool labels.Labels instances, as they're immutable.

pkg/frontend/querymiddleware/results_cache.go

pkg/frontend/querymiddleware/split_and_cache_test.go

charleskorn · 2025-04-10T03:43:51Z

pkg/frontend/querymiddleware/split_and_cache_test.go

@@ -1014,6 +1036,7 @@ func TestSplitAndCacheMiddleware_ResultsCacheFuzzy(t *testing.T) {
 }

 func TestSplitAndCacheMiddleware_ResultsCache_ExtentsEdgeCases(t *testing.T) {
+	t.Skip()


Why is this test skipped?

charleskorn · 2025-04-10T03:49:53Z

pkg/frontend/querymiddleware/spin_off_subqueries_queryable.go

+		// FIXME: newSeriesSetFromEmbeddedQueriesResults copies samples, but does not do a deep copy of histogram spans.
+		// We either need to do a deep copy of the histogram spans or somehow wrap the SeriesSets with a closer.
+		// Labels also need closer inspection as they appear to be pointers too.


That coupled with the possibility that any change here may be short lived after the introduction of the planner.

I think this is the deciding factor for me, and the fact that histograms aren't yet widely used - so I'm OK with the idea of doing a deep copy of histograms.

Labels also need closer inspection as they appear to be pointers too.

Can you elaborate on this?

They are closed downstream

charleskorn · 2025-04-15T00:45:51Z

pkg/frontend/querymiddleware/codec_json_test.go

+	require.Equal(t, expected.ContentLength, actual.ContentLength)
+
+	// Verify that body types match
+	require.Equal(t, reflect.TypeOf(expected.Body), reflect.TypeOf(actual.Body))


Why do we need to compare the body types? Can't we just confirm they contain the same or equivalent content?

charleskorn · 2025-04-15T00:49:49Z

pkg/frontend/querymiddleware/results_cache.go

@@ -146,7 +146,10 @@ type PrometheusResponseExtractor struct{}

 // Extract extracts response for specific a range from a response.
 func (PrometheusResponseExtractor) Extract(start, end int64, from Response) Response {


Should we expand the comment here to clarify whose responsibility it is to close resp, and how this relates to the returned Response? (eg. does closing resp also close the returned value?)

(same comment applies to ResponseWithoutHeaders)

charleskorn · 2025-04-15T00:51:05Z

pkg/frontend/querymiddleware/split_and_cache_test.go

@@ -1698,6 +1729,9 @@ func (q roundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
 	if err != nil {
 		return nil, err
 	}
+	// TODO Check if it's safe to close the response here. I suspect EncodeMetricsQueryResponse does not do a deep copy and thus it is not yet safe.


Is this TODO still relevant?

charleskorn · 2025-04-15T00:51:44Z

pkg/frontend/querymiddleware/spin_off_subqueries_queryable.go

@@ -179,9 +185,14 @@ func (q *spinOffSubqueriesQuerier) Select(ctx context.Context, _ bool, hints *st
 			if err != nil {
 				return storage.ErrSeriesSet(fmt.Errorf("error running subquery: %w", err))
 			}
-			promRes, ok := resp.(*PrometheusResponse)
+			// FIXME: newSeriesSetFromEmbeddedQueriesResults copies samples, but does not do a deep copy of histogram spans.


Does this FIXME still apply?

jhesketh added 4 commits April 9, 2025 16:17

query-frontend: Ensure queries are properly closed

cce0f19

Make sure for each execution of a query we properly call the Close() interface. This is necessary where an engine (such as MQE) pools result slices. Fixes: grafana#5713

Fix lint

23d2419

Close query as part of the http Response closer

5d5424d

Update fixme comments

e71eb7b

jhesketh commented Apr 9, 2025

View reviewed changes

charleskorn reviewed Apr 10, 2025

View reviewed changes

jhesketh added 5 commits April 13, 2025 21:28

Do not prematurely close accumulators

96463b8

They are closed downstream

Correctly compare http body types

43f1e95

Only marshal the promethues response

7403b95

Fix remaining test

67a7b0e

Fix lint

c926d0e

jhesketh marked this pull request as ready for review April 14, 2025 12:14

jhesketh requested a review from a team as a code owner April 14, 2025 12:14

Merge branch 'main' into jhesketh/mqe-frontends3

0f5d878

charleskorn reviewed Apr 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

query-frontend: Ensure queries are properly closed #11157

query-frontend: Ensure queries are properly closed #11157

jhesketh commented Apr 9, 2025 •

edited

Loading

jhesketh Apr 9, 2025

charleskorn Apr 10, 2025

jhesketh Apr 13, 2025

charleskorn Apr 13, 2025

jhesketh Apr 14, 2025

charleskorn Apr 15, 2025

charleskorn Apr 10, 2025

charleskorn Apr 10, 2025

charleskorn Apr 15, 2025

charleskorn Apr 15, 2025

charleskorn Apr 15, 2025

charleskorn Apr 15, 2025

		@@ -146,7 +146,10 @@ type PrometheusResponseExtractor struct{}

		// Extract extracts response for specific a range from a response.
		func (PrometheusResponseExtractor) Extract(start, end int64, from Response) Response {

query-frontend: Ensure queries are properly closed #11157

Are you sure you want to change the base?

query-frontend: Ensure queries are properly closed #11157

Conversation

jhesketh commented Apr 9, 2025 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhesketh commented Apr 9, 2025 •

edited

Loading