Skip to content

Conversation

@SungJin1212
Copy link
Member

This PR adds a fallback logic to the thanos-promql engine.
FYI. The current version of the thanos-promql supports fallback logic, so there is no user impact.

Which issue(s) this PR fixes:
Fixes #6624

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@SungJin1212 SungJin1212 force-pushed the Add-thanos-engine-fallback branch from dc78802 to 8af8936 Compare March 5, 2025 07:45
@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 5, 2025
@SungJin1212 SungJin1212 force-pushed the Add-thanos-engine-fallback branch from 8af8936 to bbc0cc1 Compare March 5, 2025 07:47
@SungJin1212 SungJin1212 requested a review from yeya24 March 5, 2025 08:03
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work

if qf.thanosEngine != nil {
res, err := qf.thanosEngine.MakeInstantQuery(ctx, q, fromPromQLOpts(opts), qs, ts)
if err != nil {
if engine.IsUnimplemented(err) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a counter metric for number of fallbacks because new engine doesn't support it?
Similar to
https://github.com/thanos-io/promql-engine/pull/518/files#diff-2e6c4934f63ff9b712c2c346b33036af4724adf70b0801fff9b74f71b37fcd89L180

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I add a metric and update the pr.

@SungJin1212 SungJin1212 force-pushed the Add-thanos-engine-fallback branch 2 times, most recently from 1dcc56d to c37ece7 Compare March 6, 2025 01:37
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a unit test at least?

@SungJin1212 SungJin1212 force-pushed the Add-thanos-engine-fallback branch from c37ece7 to 3f2a618 Compare March 6, 2025 04:31
@SungJin1212 SungJin1212 requested a review from yeya24 March 6, 2025 04:32
@SungJin1212 SungJin1212 force-pushed the Add-thanos-engine-fallback branch from 3f2a618 to 8762531 Compare March 6, 2025 04:37
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@yeya24 yeya24 merged commit 3e4f186 into cortexproject:master Mar 6, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement thanos promql engine fallback logic

2 participants