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

Convert all validation.Limits durations to model.Duration #4044

Merged
merged 5 commits into from
Apr 2, 2021
Merged

Convert all validation.Limits durations to model.Duration #4044

merged 5 commits into from
Apr 2, 2021

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Apr 1, 2021

What this PR does:

Use the prometheus/common model.Duration for all duration related
limits instead of the standard library time.Duration type. This works
around an issue with the JSON representation of the standard library
type.

For example:

  • The YAML representation of time.Duration(1 * time.Second) is the string 1s
  • The JSON representation of time.Duration(1 * time.Second) is the integer 1000000000

By contrast:

  • The YAML representation of model.Duration(1 * time.Second) is the string 1s
  • The JSON representation of model.Duration(1 * time.Second) is the string 1s

The first commit in this PR demonstrates this issue with a unit test.

Note that the Overrides struct still converts model.Duration to time.Duration
for each respective limit method (the same way MaxQueryLookback already
was).

Signed-off-by: Nick Pillitteri [email protected]

Checklist

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

Use the prometheus/common model.Duration for all duration related
limits instead of the standard library time.Duration type. This works
around an issue with the JSON representation of the standard library
type.

For example:
* The YAML representation of time.Duration(1 * time.Second) is the string `1s`
* The JSON representation of time.Duration(1 * time.Second) is the integer `1000000000`

By contrast:
* The YAML representation of model.Duration(1 * time.Second) is the string `1s`
* The JSON representation of model.Duration(1 * time.Second) is the string `1s`

Signed-off-by: Nick Pillitteri <[email protected]>
@56quarters 56quarters marked this pull request as ready for review April 1, 2021 22:40
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

Good work and thanks for looking into this.

Co-authored-by: Jacob Lisi <[email protected]>
Signed-off-by: Nick Pillitteri <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM (modulo a nit).

Signed-off-by: Nick Pillitteri <[email protected]>
@pracucci pracucci merged commit cb8ed0e into cortexproject:master Apr 2, 2021
@56quarters 56quarters deleted the json-duration branch April 2, 2021 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants