-
Notifications
You must be signed in to change notification settings - Fork 835
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
base: master
Are you sure you want to change the base?
Refactor query api #6779
Conversation
722b8dc to
fb6f4c0
Compare
dd095a9 to
af9b1be
Compare
|
@harry671003 Can you please help take a look at this PR? |
| package query | ||
|
|
||
| // Response defines the Prometheus response format. | ||
| type Response struct { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
pkg/util/api/parse.go
Outdated
| maxTimeFormatted = MaxTime.Format(time.RFC3339Nano) | ||
| ) | ||
|
|
||
| func ExtractQueryOpts(r *http.Request) (promql.QueryOpts, error) { |
There was a problem hiding this comment.
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
pkg/util/api/parse.go
Outdated
| return 0, httpgrpc.Errorf(http.StatusBadRequest, "%s", err.Error()) | ||
| } | ||
|
|
||
| return util.TimeToMillis(t), nil |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
tripperwareuses original functions that return ahttperror, and then wrapshttp errorwithgrpcand convertstimestruct tounix ts. - The
api utildefines functions that returngrpcerror andunix ts(this way). - The
tripperwarereuses the original functions and changesPrometheusRequest'sint64type (unix ts) totimestruct. It requires a change to existing functions.
WDTY?
There was a problem hiding this comment.
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.
521a7dc to
850336a
Compare
850336a to
5320d89
Compare
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
5320d89 to
8c4b193
Compare
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
queryapiandtripperwareto the api util package.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]