Skip to content
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

query-frontend: remove duplicate defaulting of instant query time param #7026

Conversation

dimitarvdimitrov
Copy link
Contributor

After #6985 the default value of the time parameter (i.e. time.Now()) is set in the PrometheusCodec. This PR removes the default set in an HTTP middleware.

With this change the time parameter in the HTTP request is no longer populated if the client didn't provide it. Instead, it will only be populated in the decoded querymiddleware.Request. The HTTP parameter was referenced in a few places

…aram

After 6985 the default value of the time parameter (i.e. time.Now()) is set in the PrometheusCodec. This PR removes the
default set in an HTTP middleware.

With this change the time parameter in the HTTP request is no longer populated if
the client didn't provide it. Instead, it will only be populated in the decoded `querymiddleware.Request`. The HTTP parameter was referenced in a few places
* [DecodeInstantQueryTimeParams](https://github.com/grafana/mimir/blob/9fd2b19a61acec59336d677a50270806dcc4482c/pkg/frontend/querymiddleware/codec.go#L295) which now also falls back to the default (after PR 6985). This is also used for partial queries, but those shouldn't observe an unset parameter (since the parent query had the parameter defaulted to `time.Now()`).
* invocations of [ParseTimeParam](https://github.com/grafana/mimir/blob/bcb5c345e03a7c8d0ff0a2e0985be448a332dd7d/pkg/util/time.go#L40). One of them is in DecodeInstantQueryTimeParams, the other two are for label values requests, which have their own logic for defaulting the parameters.

Signed-off-by: Dimitar Dimitrov <[email protected]>
Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

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

Thank you. Can you also open a PR for removing this from GEM?

@dimitarvdimitrov dimitarvdimitrov enabled auto-merge (squash) January 4, 2024 12:22
@dimitarvdimitrov dimitarvdimitrov merged commit 4c30771 into main Jan 4, 2024
28 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/query-frontend/remove-multiple-query-time-defaults branch January 4, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants