Skip to content

Add dedicated instant/range query handlers #6763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add dedicated instant/range query handlers #6763

wants to merge 1 commit into from
+710 −46

Conversation

SungJin1212
Copy link
Member

@SungJin1212 SungJin1212 commented May 26, 2025 •
edited
Loading

This PR adds dedicated instant/range query handlers to customize these APIs.

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

Checklist

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

@SungJin1212 SungJin1212 force-pushed the Add-custom-instant/range-handlers branch from 721af3a to 5c890a8 Compare May 26, 2025 02:52
yeya24
yeya24 reviewed May 26, 2025
View reviewed changes
pkg/api/custom_api.go Outdated
}

// Custom handler for Query range API
func (c *CustomAPI) rangeQueryHandler(r *http.Request) (result apiFuncResult) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest we move query handlers implementation to its own package in pkg/querier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can think of reusing this in queryfrontend in the future. Can we keep in pkg/api/query?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker for this PR. Should we do this for all routes like Thanos does?

Copy link
Contributor

Choose a reason for hiding this comment

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

@SungJin1212 Can we change the file path as Harry suggested?

@SungJin1212 SungJin1212 force-pushed the Add-custom-instant/range-handlers branch 6 times, most recently from 98f5859 to 8007a94 Compare May 26, 2025 04:42
@SungJin1212 SungJin1212 marked this pull request as draft May 26, 2025 05:13
@SungJin1212
Copy link
Member Author

There is an issue in the test. I'm fixing.

yeya24
yeya24 reviewed May 26, 2025
View reviewed changes
@SungJin1212 SungJin1212 force-pushed the Add-custom-instant/range-handlers branch 2 times, most recently from fed32aa to b2820ce Compare May 26, 2025 09:25
@SungJin1212
Copy link
Member Author

SungJin1212 commented May 26, 2025 •
edited
Loading

@yeya24
I'm modifying the code to reuse existing functions and to emit the gRPC error.

@SungJin1212 SungJin1212 force-pushed the Add-custom-instant/range-handlers branch 2 times, most recently from 835cfee to 062bc9b Compare May 27, 2025 02:57
@SungJin1212 SungJin1212 requested a review from yeya24 May 27, 2025 05:23
@SungJin1212 SungJin1212 force-pushed the Add-custom-instant/range-handlers branch from 062bc9b to 2e95f38 Compare May 27, 2025 05:25
@SungJin1212 SungJin1212 marked this pull request as ready for review May 27, 2025 05:46
@SungJin1212 SungJin1212 force-pushed the Add-custom-instant/range-handlers branch from 2e95f38 to b0871cb Compare May 27, 2025 10:21
yeya24
yeya24 reviewed May 28, 2025
View reviewed changes
@yeya24
Copy link
Contributor

yeya24 commented May 28, 2025

I'm modifying the code to reuse existing functions and to emit the gRPC error.

@SungJin1212 Thanks for this. It is ok for now. I wonder if it makes more sense if we keep the original parse function which returns basic errors in the API code. And query frontend handler can import those functions and wrap with gRPC errors instead.

This way seems cleaner and we don't have to respond with gRPC errors in the query API handler

@SungJin1212
Copy link
Member Author

@yeya24

I wonder if it makes more sense if we keep the original parse function which returns basic errors in the API code. And query frontend handler can import those functions and wrap with gRPC errors instead.

Yeah, I think it would be better if the query API could emit a basic error. But can I do that in the next refactoring PR? It seems that many codes need to be changed.

@SungJin1212 SungJin1212 force-pushed the Add-custom-instant/range-handlers branch 2 times, most recently from d9379b5 to 6a6ca74 Compare May 28, 2025 02:57
@SungJin1212 SungJin1212 requested a review from yeya24 May 29, 2025 00:09
yeya24
yeya24 approved these changes May 29, 2025
View reviewed changes
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. This LGTM. Let's at least get another approval before merging it

@yeya24
Copy link
Contributor

yeya24 commented May 29, 2025

signal: killed
FAIL github.com/cortexproject/cortex/pkg/ingester 100.282s

We should probably do something to fix the OOM kill

harry671003
harry671003 reviewed May 29, 2025
View reviewed changes
pkg/querier/queryapi/query_api.go Outdated
@@ -0,0 +1,249 @@
package queryapi
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in the api package?
Here's how Thanos does this for a reference.
https://github.com/thanos-io/thanos/tree/main/pkg/api

harry671003
harry671003 reviewed May 29, 2025
View reviewed changes
@SungJin1212 SungJin1212 force-pushed the Add-custom-instant/range-handlers branch from 6a6ca74 to 9445d27 Compare May 30, 2025 01:16
@SungJin1212 SungJin1212 requested a review from harry671003 May 30, 2025 02:04
@alanprot
Copy link
Member

I think this make sense! i would just reference the prometheus query handler on query api file so we know from where it was based of, so we can easily bring new thing that are added there.

@alanprot
Copy link
Member

Why do only for query though? if we are doing this is not better to do for everything? (GetLabelsNames/Values for instance as well?)

@yeya24
Copy link
Contributor

yeya24 commented May 30, 2025

Why do only for query though? if we are doing this is not better to do for everything? (GetLabelsNames/Values for instance as well?)

We can do it in the future. I don't see any requirement to implement other APIs today.

@SungJin1212
Add dedicated instant/range query handlers
0f50d21 
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
@SungJin1212 SungJin1212 force-pushed the Add-custom-instant/range-handlers branch from 9445d27 to 0f50d21 Compare May 31, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeya24 yeya24 yeya24 approved these changes

@harry671003 harry671003 harry671003 approved these changes

Assignees
No one assigned
Labels
component/querier size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create dedicated query and query range API handlers
4 participants
@SungJin1212 @yeya24 @alanprot @harry671003