unify time parsing logic in prometheus codec; clarify labels-series compatibility #10117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
francoposa
changed the title
francoposa
force-pushed
the
francoposa/prometheus-codec-separate-labels-and-series-validation
branch
from
a8aee76 to
4897225
Compare
|
related PR: Early Request Validation in Query Frontend #10093 |
dimitarvdimitrov
approved these changes
Dec 5, 2024
| @@ -531,10 +536,10 @@ func DecodeRangeQueryTimeParams(reqValues *url.Values) (start, end, step int64, | |||
| } | |||
|
|
|||
| func instantTimeParamNow() int64 { | |||
| return time.Now().UnixMilli() | |||
| return time.Now().UTC().UnixMilli() | |||
There was a problem hiding this comment.
is the UTC() necessary? I thought UnixMilli is only defined in UTC
There was a problem hiding this comment.
Ah yeah true. I am just in the habit of making sure anything with time gets UTC-ed
francoposa
force-pushed
the
francoposa/prometheus-codec-separate-labels-and-series-validation
branch
from
35830da to
a880464
Compare
francoposa
deleted the
francoposa/prometheus-codec-separate-labels-and-series-validation
branch
bjorns163
pushed a commit
to bjorns163/mimir
that referenced
this pull request
Dec 30, 2024
…ompatibility (grafana#10117) * unify time parsing logic in prometheus codec; clarify labels-series req
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does
With an eye towards early request validation, I set out to implement endpoint param parsing for the series endpoints.
To start, this unifies time parsing logic using the
promTimeParamDecodabletype.This could be reviewed on its own but I got a bit carried away.
I also chased down that we have already implemented this for the series endpoint, it was just unclear from the interface because everything is called a "labels request".
I have renamed types to reflect the fact that they are for both labels and series, as well as added comments on
DecodeLabelsSeriesQueryTimeParamsabout how the Prometheus HTTP API spec makes it appear as though the labels and series start/end processing is incompatible, but it actually is compatible per the implementation.Renaming is a bit disruptive but I prefer it to just leaving misleading naming. In any case, if we were to pick one name to cover "Labels and Series" requests, the more general and accurate name would be Series.
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.