[query] Allow customizing request parsing for /query handler #2884
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2884 +/- ##
========================================
- Coverage 71.9% 71.9% -0.1%
========================================
Files 1096 1096
Lines 99792 99781 -11
========================================
- Hits 71844 71830 -14
- Misses 23028 23031 +3
Partials 4920 4920
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
robskillington
force-pushed
the
master
branch
from
e130b7e to
3cf877e
Compare
robskillington
force-pushed
the
vilius/custom_read_request_parser
branch
from
364f205 to
c6f43b2
Compare
| @@ -63,6 +76,18 @@ func NewReadInstantHandler(opts Options, hOpts options.HandlerOptions) http.Hand | |||
| return newReadInstantHandler(opts, hOpts, queryable) | |||
| } | |||
|
|
|||
| // DefaultReadRequestParser returns the default function that parse read request arguments | |||
There was a problem hiding this comment.
| // DefaultReadRequestParser returns the default function that parse read request arguments | |
| // DefaultReadRequestParser returns the default function that parses read request arguments. |
There was a problem hiding this comment.
| @@ -39,15 +39,20 @@ import ( | |||
| "go.uber.org/zap" | |||
| ) | |||
|
|
|||
| // RequestParser is a function that parses request arguments | |||
There was a problem hiding this comment.
| @@ -63,6 +76,18 @@ func NewReadInstantHandler(opts Options, hOpts options.HandlerOptions) http.Hand | |||
| return newReadInstantHandler(opts, hOpts, queryable) | |||
| } | |||
|
|
|||
| // DefaultReadRequestParser returns the default function that parse read request arguments | |||
There was a problem hiding this comment.
| // DefaultReadRequestParser returns the default function that parse read request arguments | |
| // DefaultReadRequestParser returns the default function that parses read request arguments. |
| @@ -63,6 +76,18 @@ func NewReadInstantHandler(opts Options, hOpts options.HandlerOptions) http.Hand | |||
| return newReadInstantHandler(opts, hOpts, queryable) | |||
| } | |||
|
|
|||
| // DefaultReadRequestParser returns the default function that parse read request arguments | |||
| func DefaultReadRequestParser(opts options.HandlerOptions) RequestParser { | |||
There was a problem hiding this comment.
hOpts(for consistency with how options.HandlerOptions is called in another function).
There was a problem hiding this comment.
| @@ -39,15 +39,20 @@ import ( | |||
| "go.uber.org/zap" | |||
| ) | |||
|
|
|||
| // RequestParser is a function that parses request arguments | |||
| type RequestParser func(ctx context.Context, r *http.Request) (models.RequestParams, error) | |||
There was a problem hiding this comment.
If you are planning to have more extension points like this (eg. ResponseHandler and so on), might be better to introduce an interface for them, so that you would have to inject a single thing.
There was a problem hiding this comment.
Good point. Maybe existing Options could be used for that?
m3/src/query/api/v1/handler/prom/prom.go
Lines 41 to 44 in dd58747
E.g. add a field for read request parser and create a function NewOptions(), which would set the default parser from the get-go. The latter could be changed on demand.
opts := prom.NewOptions(promEngine)
defaultParser := opts.ReadRequestParser
opts.ReadRequestParser = func(/* ... */) /* ... */ {
params, err := defaultParser(/* ... */)
// modify params
return params, err
}Should be easy to add additional ways to configure the handler in a backwards compatible way. We could also get rid of NewReadHandlerWithCustomParser function.
There was a problem hiding this comment.
Ideally Options would be immutable (see other use cases).
I suggest to discuss the approach with @soundvibe .
There was a problem hiding this comment.
BTW there is an example of what I meant in my suggestion: https://github.com/m3db/m3/blob/2c170ebd9c4acf6f132213eb07bf7c87a1077b14/src/dbnode/storage/types.go#L1393-L1397
Not sure how applicable to your case, but this would be more convenient than function injecting from the point where you have to inject more than a single function.
There was a problem hiding this comment.
Something like this? 1031323#diff-cbccca18d3e7a3ac7e3867b53f6ed3673949a973ba1ddf3c882f721f81c842aaR43-R51
There was a problem hiding this comment.
Yes, something like that. But please treat it just as a suggesstion - you know your use case in full picture better than I do.
What this PR does / why we need it:
Allows customizing request parsing when handling PromQL's
api/v1/queryrequestsSpecial notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: