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.

Sign up for GitHub

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] Fix step parsing #1617

Merged
merged 9 commits into from
May 9, 2019
Merged

[query] Fix step parsing #1617

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8c646c6
Fix step parsing
May 8, 2019
748f104
Change comment
May 8, 2019
1f76b76
Add overflow error test
May 8, 2019
15316b3
String interpolation
May 8, 2019
ef0e3fb
Address comments
May 8, 2019
2bb2954
Store as const
May 8, 2019
87b4d3d
Check negative step
May 9, 2019
e86a4b7
Update error
May 9, 2019
2632fce
Merge branch 'master' into braskin/investigate_1610
May 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/query/api/v1/handler/prometheus/native/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,13 @@ func parseDuration(r *http.Request, key string) (time.Duration, error) {
}

// Try parsing as an integer value specifying seconds, the Prometheus default
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Update comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ha beat you to it :)

if seconds, intErr := strconv.ParseInt(str, 10, 64); intErr == nil {
return time.Duration(seconds) * time.Second, nil
if seconds, intErr := strconv.ParseFloat(str, 64); intErr == nil {
ts := seconds * float64(time.Second)
if ts > float64(math.MaxInt64) || ts < float64(math.MinInt64) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it valid for duration to be negative in the prom context? If not, maybe check that it's positive instead of smaller than minInt64?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, it might be worth storing the float version of maxInt64 as a const so you don't need to cast each time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what Prometheus checks, so tempted to just leave it.

https://github.com/prometheus/prometheus/blob/master/web/api/v1/api.go#L1171

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, it might be worth storing the float version of maxInt64 as a const so you don't need to cast each time.

Sounds good.

return 0, fmt.Errorf("cannot parse %q to a valid duration. It overflows int64", str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: cannot parse %q to a valid duration: int64 overflow or similar (errors aren't sentences)

}

return time.Duration(ts), nil
}

return 0, err
Expand Down
16 changes: 16 additions & 0 deletions src/query/api/v1/handler/prometheus/native/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,14 @@ func TestParseDuration(t *testing.T) {
assert.Equal(t, 10*time.Second, v)
}

func TestParseFloatDuration(t *testing.T) {
r, err := http.NewRequest(http.MethodGet, "/foo?step=10.00m", nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prob want a test where the step is not just a round number? eg Test to ensure that /foo?step=10.85m return the expected output, whether that's the precise duration, a rounded duration or an error.

require.NoError(t, err)
v, err := parseDuration(r, stepParam)
require.NoError(t, err)
assert.Equal(t, 10*time.Minute, v)
}

func TestParseDurationParsesIntAsSeconds(t *testing.T) {
r, err := http.NewRequest(http.MethodGet, "/foo?step=30", nil)
require.NoError(t, err)
Expand All @@ -120,6 +128,14 @@ func TestParseDurationParsesIntAsSeconds(t *testing.T) {
assert.Equal(t, 30*time.Second, v)
}

func TestParseDurationParsesFloatAsSeconds(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a case for "/foo?step=30"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is one already.

r, err := http.NewRequest(http.MethodGet, "/foo?step=30.00", nil)
require.NoError(t, err)
v, err := parseDuration(r, stepParam)
require.NoError(t, err)
assert.Equal(t, 30*time.Second, v)
}

func TestParseDurationError(t *testing.T) {
r, err := http.NewRequest(http.MethodGet, "/foo?step=bar10", nil)
require.NoError(t, err)
Expand Down