Skip to content

Refactor query api #6779

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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SungJin1212
Copy link
Member

@SungJin1212 SungJin1212 commented Jun 2, 2025

The PR #6763 added dedicated instant/range query handlers. This PR changes the handlers to emit the original error. Also, it moves common functions used in queryapi and tripperware to the api util package.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@SungJin1212 SungJin1212 force-pushed the Refactor-query-api branch from 722b8dc to fb6f4c0 Compare June 2, 2025 06:52
@SungJin1212 SungJin1212 marked this pull request as draft June 2, 2025 06:53
@SungJin1212 SungJin1212 force-pushed the Refactor-query-api branch from dd095a9 to af9b1be Compare June 2, 2025 10:41
@SungJin1212 SungJin1212 marked this pull request as ready for review June 2, 2025 10:57
@yeya24
Copy link
Contributor

yeya24 commented Jun 3, 2025

@harry671003 Can you please help take a look at this PR?

package query

// Response defines the Prometheus response format.
type Response struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we cannot reuse the one from util/api/response.go?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to use the original code.. The v1.ErrorType doesn't have a value like ErrNotFound and ErrInternal.

maxTimeFormatted = MaxTime.Format(time.RFC3339Nano)
)

func ExtractQueryOpts(r *http.Request) (promql.QueryOpts, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes more sense to move this to query API

return 0, httpgrpc.Errorf(http.StatusBadRequest, "%s", err.Error())
}

return util.TimeToMillis(t), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Umm, I dislike current implementation as we are still keeping 2 copies of code. Then it doesn't make much sense to put them in a common place for reuse.
And it is weird to have one function returns standard error and another returns httpgrpc error.

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 was thinking these ways.

  • The tripperware uses original functions that return a http error, and then wraps http error with grpc and converts time struct to unix ts.
  • The api util defines functions that return grpc error and unix ts (this way).
  • The tripperware reuses the original functions and changes PrometheusRequest's int64 type (unix ts) to time struct. It requires a change to existing functions.

WDTY?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we define two types of functions and each type of function is either used by tripperware or query API then I would suggest we put them into separate packages rather than the same places... If they are not re-used anyway.

@SungJin1212 SungJin1212 force-pushed the Refactor-query-api branch 3 times, most recently from ca9f865 to 521a7dc Compare June 5, 2025 07:28
Signed-off-by: SungJin1212 <[email protected]>
Signed-off-by: SungJin1212 <[email protected]>
@SungJin1212 SungJin1212 force-pushed the Refactor-query-api branch from 521a7dc to 850336a Compare June 9, 2025 01:24
@SungJin1212 SungJin1212 requested a review from yeya24 June 9, 2025 21:01
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.

2 participants