feat: implement query splitting in the query-tee

[trevorwhitney]

What this PR does / why we need it:

This PR introduces query splitting in the query-tee, giving us to split queries into recent and old splits. This is valuable since the new Thor architecture cannot handle queries for recent data yet, and, as a result, means we don't want to compare queries with recent data between the new and old architecture. By splitting off the recent data part of the query into it's own partition, we can then only send the old data splits to both backends for comparison.

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here
  • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

[Copilot]

Pull request overview

This PR implements query splitting functionality in the query-tee to enable gradual migration to the Thor architecture. The implementation splits queries into "recent" (data newer than a threshold) and "old" (data older than the threshold) segments. Recent data is only fetched from the preferred backend, while old data is sent to both backends for Goldfish comparison, since Thor cannot yet handle recent data queries.

Key changes:

• Introduces time-based query splitting logic with step-aligned split points to avoid data overlap
• Adds concatenation logic for matrix (metrics) and stream (logs) responses with direction-aware merging
• Integrates splitting into the proxy endpoint with configurable comparison_min_age threshold

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
tools/querytee/query_splitter.go	Core implementation of query splitting, step alignment, and response concatenation logic
tools/querytee/query_splitter_test.go	Comprehensive test coverage for splitting logic and response concatenation
tools/querytee/proxy_endpoint.go	Integration of query splitting with goldfish comparison flow and parallel execution
tools/querytee/proxy_endpoint_test.go	Integration tests verifying split query behavior across different scenarios
tools/querytee/goldfish/config.go	Adds comparison_min_age configuration option for controlling split threshold
tools/querytee/goldfish/manager.go	Exposes ComparisonMinAge() accessor and exports ExtractUserFromQueryTags function
tools/querytee/goldfish/manager_test.go	Updates test to use exported function name
pkg/querier/queryrange/codec.go	Exports DecodeResponseJSONFrom for use in query splitting
pkg/loghttp/params.go	Exports ParseTimestamp function for external use
pkg/loghttp/params_test.go	Adds comprehensive test coverage for ParseTimestamp function

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Incorrect assertion format. The assertion message format has the format string inside assert.Equal, but the variadic args are outside. This should be using require.Equalf or assert.Equalf instead, or the error message should be simplified to not include format specifiers. As written, the %d won't be replaced with the actual value.

Suggested change

	assert.Equal(t, 3, len(receivedQueries), "expected 3 queries, 1 for the recent portion, and 1 to each backend for the old portion, got %d", len(receivedQueries))
	assert.Equalf(t, 3, len(receivedQueries), "expected 3 queries, 1 for the recent portion, and 1 to each backend for the old portion, got %d", len(receivedQueries))

[Copilot]

There's a TODO comment that should be removed or tracked in an issue. TODOs in production code should generally reference issue tracking numbers or be resolved before merge.

Suggested change

// TODO(twhitney): this will no longer be an error when we allow racing

[Copilot]

The error returned here uses the wrong variable. It uses err from the outer scope (which is from concatenateResponses), but should create a new error specific to this failure case. This could lead to confusing error messages if err happens to be nil or contains an unrelated error.

Suggested change

	err: fmt.Errorf("preferred backend old data split failed: %w", err),
	err: fmt.Errorf("preferred backend old data split failed: %w", preferredOldRes.err),

[Copilot]

Using panic() in production code is problematic. This should return an error or log a critical error instead. Panics should be reserved for truly unrecoverable situations, and a length mismatch between responses and backends could be handled more gracefully (e.g., by logging an error and returning early).

Suggested change

	panic("responses and backends must have the same length")
	level.Error(ex.logger).Log(
	"msg", "responses and backends must have the same length",
	"responses_len", len(responses),
	"backends_len", len(backends),
	"issuer", ex.issuer,
	)
	return

[Copilot]

Potential goroutine leak: The nested goroutine on line 386 waits for allBackendsWg but may never complete if any backend request in the wait group never finishes (e.g., due to a timeout or panic). Additionally, if this goroutine is blocked waiting and resCh has already been closed, there's no cancellation mechanism to stop it. Consider adding a context with timeout or a way to cancel this background processing if the parent goroutine exits early due to errors.

[Copilot]

Creating a new background context here discards the original request context which may contain important information like cancellation signals, trace IDs, or deadlines. Consider using r.Context() instead of context.Background() to preserve the request's context chain. This ensures that if the client disconnects or the request times out, the decoding/encoding operations are also cancelled.

Suggested change

	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	ctx, cancel := context.WithTimeout(r.Context(), 30*time.Second)

[Copilot]

Using time.Sleep() to wait for asynchronous processing in tests is a flaky pattern. This test could fail intermittently on slow systems or pass when it should fail on fast systems. Consider using synchronization primitives (channels, waitgroups) or polling with a timeout to wait for the expected condition instead.

[Copilot]

[nitpick] Inconsistent documentation terminology: The comment says "should not be sent to goldfish at all" but Goldfish is the comparison tool name, not the backend. The comment should clarify that the query shouldn't be sent to both backends for Goldfish comparison, only to the preferred backend for serving.

Suggested change

	// If true, the query should not be sent to goldfish at all.
	// If true, the query should not be sent to both backends for Goldfish comparison, but only to the preferred backend for serving.

[Copilot]

Typo in function comment: "ComparsionMinAge" should be "ComparisonMinAge".

Suggested change

	// ComparsionMinAge returns the minimum age of data to send to goldfish for comparison.
	// ComparisonMinAge returns the minimum age of data to send to goldfish for comparison.

[Copilot]

There's a TODO comment that should be removed or tracked in an issue. TODOs in production code should generally reference issue tracking numbers or be resolved before merge.

Suggested change

	// TODO(twhitney): this will change when we implement racing
	// TODO(twhitney): this will change when we implement racing. See issue #1234

[rfratto]

When we move to the racing model, we should continue here instead of returning the error, and move the spawn of the remaining to when we're definitely returning

[rfratto]

When we move to racing, we might want to return the error from the preferred backend instead of the first response (maybe?)