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 6 commits
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
14 changes: 11 additions & 3 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 @@ -53,6 +53,9 @@ const (
blockTypeParam = "block-type"

formatErrStr = "error parsing param: %s, error: %v"

maxInt64 = float64(math.MaxInt64)
minInt64 = float64(math.MinInt64)
)

func parseTime(r *http.Request, key string) (time.Time, error) {
Expand All @@ -75,9 +78,14 @@ func parseDuration(r *http.Request, key string) (time.Duration, error) {
return value, nil
}

// Try parsing as an integer value specifying seconds, the Prometheus default
if seconds, intErr := strconv.ParseInt(str, 10, 64); intErr == nil {
return time.Duration(seconds) * time.Second, nil
// Try parsing as a float value specifying seconds, the Prometheus default
if seconds, floatErr := strconv.ParseFloat(str, 64); floatErr == nil {
Copy link
Collaborator

@arnikola arnikola May 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think there's a bug here, you're not returning floatErr here if it's not nil

also, nit: when you have a decent size codeblock I think it might read a little nicer to flip the conditional and then split this up, i.e.

seconds, err := strconv.ParseFloat(str, 64)
if err != nil {
 return 0, err
}
// logic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eh i mean it only gets to here if this returns an error:

	value, err := time.ParseDuration(str)

So either way it'll return an error - just might not be the float conversion error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's wrong. It needs to return this error

ts := seconds * float64(time.Second)
if ts > maxInt64 || ts < minInt64 {
benraskin92 marked this conversation as resolved.
Show resolved Hide resolved
return 0, fmt.Errorf("cannot parse %s to a valid duration: int64 overflow", str)
}

return time.Duration(ts), nil
}

return 0, err
Expand Down
30 changes: 30 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 @@ -23,6 +23,7 @@ package native
import (
"bytes"
"encoding/json"
"fmt"
"math"
"net/http"
"net/url"
Expand Down Expand Up @@ -112,6 +113,20 @@ 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.85m", nil)
require.NoError(t, err)
v, err := parseDuration(r, stepParam)
require.NoError(t, err)
assert.Equal(t, 10*time.Minute+51*time.Second, v)

r, err = http.NewRequest(http.MethodGet, "/foo?step=10.00m", nil)
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,13 +135,28 @@ 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)
_, err = parseDuration(r, stepParam)
assert.Error(t, err)
}

func TestParseDurationOverflowError(t *testing.T) {
r, err := http.NewRequest(http.MethodGet, fmt.Sprintf("/foo?step=%f", float64(math.MaxInt64)), nil)
require.NoError(t, err)
_, err = parseDuration(r, stepParam)
assert.Error(t, err)
}

func TestParseBlockType(t *testing.T) {
logging.InitWithCores(nil)

Expand Down