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

Add support for more user-friendly duration input #246

Merged
merged 4 commits into from
Aug 3, 2020

Conversation

roidelapluie
Copy link
Member

With this pull request, it is possible to write:

6d23h

instead of

167h

Which is a lot more easy to read.

** This is a breaking change, unless we remove the re-marshalling for now. **

Signed-off-by: Julien Pivotto roidelapluie@inuits.eu

With this pull request, it is possible to write:

6d23h

instead of

167h

Which is a lot more easy to read.

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie
Copy link
Member Author

user request: prometheus/prometheus#7681

Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

What do you mean by this being breaking?

model/time.go Outdated
@@ -191,67 +191,57 @@ func ParseDuration(durationStr string) (Duration, error) {
return 0, nil
}
matches := durationRE.FindStringSubmatch(durationStr)
if len(matches) != 3 {
fmt.Printf("%v", len(matches))
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug

model/time.go Show resolved Hide resolved
model/time.go Outdated
@@ -191,67 +191,57 @@ func ParseDuration(durationStr string) (Duration, error) {
return 0, nil
}
matches := durationRE.FindStringSubmatch(durationStr)
if len(matches) != 3 {
fmt.Printf("%v", len(matches))
if len(matches) != 15 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking for nil is the usual way

@roidelapluie
Copy link
Member Author

This would break the people consuming e.g. the rules API.

@roidelapluie
Copy link
Member Author

Mmh in fact no. We return the duration as an integer. So that's not breaking.

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie
Copy link
Member Author

I have addressed the comments and added more (relevant) tests

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie
Copy link
Member Author

I have arbitrarily decided when we marshal a duration to only marshal weeks and years if it is the exact count.

It means that 14d will be changed to 2w but 92d will stay 92d and not 13w1d.

model/time.go Outdated
}

m(2, 1000*60*60*24*365)
Copy link
Contributor

Choose a reason for hiding this comment

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

A quick comment here listing the y etc. would make things clearer, especially for anyone glancing through the code trying to find the syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@brian-brazil
Copy link
Contributor

👍

@roidelapluie roidelapluie merged commit 9c5aa1e into prometheus:master Aug 3, 2020
roidelapluie added a commit to roidelapluie/alertmanager that referenced this pull request Aug 14, 2020
- Disable HTTP2: prometheus/common#249
- Composite duration: prometheus/common#246

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
roidelapluie added a commit to roidelapluie/alertmanager that referenced this pull request Aug 14, 2020
- Disable HTTP2: prometheus/common#249
- Composite duration: prometheus/common#246

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@juliusv
Copy link
Member

juliusv commented Aug 20, 2020

I implemented the frontend side for the graph range input for this here: prometheus/prometheus#7833

simonpasquier pushed a commit to prometheus/alertmanager that referenced this pull request Aug 25, 2020
- Disable HTTP2: prometheus/common#249
- Composite duration: prometheus/common#246

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
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