[query] Fix step parsing #1617

benraskin92 · 2019-05-08T22:20:47Z

What this PR does / why we need it:

Previously, if you included a float step size with no units (e.g. 15.00), the query would fail with

"error":"error parsing param: step, error: time: missing unit in duration 15.00"

This PR allows you to add a float step size without units (and assumes it is in seconds -- equivalent to what Prometheus does)
Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

Does this PR require updating code package or user-facing documentation?:

Maybe we can add something in the docs, but I don't think it's necessary.

codecov · 2019-05-08T22:24:31Z

Codecov Report

Merging #1617 into master will increase coverage by <.1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #1617     +/-   ##
========================================
+ Coverage    71.9%   71.9%   +<.1%     
========================================
  Files         954     954             
  Lines       79170   79176      +6     
========================================
+ Hits        56966   56985     +19     
+ Misses      18478   18471      -7     
+ Partials     3726    3720      -6

Flag	Coverage Δ
#aggregator	`82.3% <ø> (-0.1%)`	⬇️
#cluster	`85.7% <ø> (ø)`	⬆️
#collector	`63.7% <ø> (ø)`	⬆️
#dbnode	`79.8% <ø> (ø)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`73.9% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.7% <ø> (ø)`	⬆️
#query	`67.1% <100%> (ø)`	⬆️
#x	`87.1% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e0830d...2632fce. Read the comment docs.

martin-mao · 2019-05-08T22:24:33Z

src/query/api/v1/handler/prometheus/native/common.go

@@ -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


Nit: Update comment.

Ha beat you to it :)

martin-mao · 2019-05-08T22:32:15Z

src/query/api/v1/handler/prometheus/native/common.go

-		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) {


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?

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

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

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

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.

martin-mao · 2019-05-08T22:33:18Z

src/query/api/v1/handler/prometheus/native/common_test.go

@@ -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)


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.

mway

lgtm modulo martin's comments

mway · 2019-05-08T22:39:52Z

src/query/api/v1/handler/prometheus/native/common.go

+	if seconds, floatErr := strconv.ParseFloat(str, 64); floatErr == nil {
+		ts := seconds * float64(time.Second)
+		if ts > float64(math.MaxInt64) || ts < float64(math.MinInt64) {
+			return 0, fmt.Errorf("cannot parse %q to a valid duration. It overflows int64", str)


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

mway

lgtm

src/query/api/v1/handler/prometheus/native/common.go

robskillington

LGTM

arnikola · 2019-05-09T15:16:37Z

src/query/api/v1/handler/prometheus/native/common.go

-	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 {


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

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.

That's wrong. It needs to return this error

arnikola · 2019-05-09T15:19:03Z

src/query/api/v1/handler/prometheus/native/common_test.go

@@ -120,13 +146,28 @@ func TestParseDurationParsesIntAsSeconds(t *testing.T) {
 	assert.Equal(t, 30*time.Second, v)
 }

+func TestParseDurationParsesFloatAsSeconds(t *testing.T) {


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

There is one already.

Fix step parsing

8c646c6

benraskin92 requested review from robskillington, arnikola and martin-mao May 8, 2019 22:22

Change comment

748f104

Benjamin Raskin added 2 commits May 8, 2019 18:28

Add overflow error test

1f76b76

String interpolation

15316b3

martin-mao reviewed May 8, 2019

View reviewed changes

mway reviewed May 8, 2019

View reviewed changes

Benjamin Raskin added 2 commits May 8, 2019 18:58

Address comments

ef0e3fb

Store as const

2bb2954

mway approved these changes May 9, 2019

View reviewed changes

martin-mao reviewed May 9, 2019

View reviewed changes

src/query/api/v1/handler/prometheus/native/common.go Show resolved Hide resolved

Benjamin Raskin added 2 commits May 9, 2019 09:40

Check negative step

87b4d3d

Update error

e86a4b7

robskillington approved these changes May 9, 2019

View reviewed changes

Merge branch 'master' into braskin/investigate_1610

2632fce

arnikola reviewed May 9, 2019

View reviewed changes

benraskin92 merged commit 582dc07 into master May 9, 2019

benraskin92 deleted the braskin/investigate_1610 branch May 9, 2019 15:53

arnikola restored the braskin/investigate_1610 branch May 9, 2019 16:08

robskillington deleted the braskin/investigate_1610 branch May 13, 2019 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Fix step parsing #1617

[query] Fix step parsing #1617

benraskin92 commented May 8, 2019

codecov bot commented May 8, 2019 •

edited

Loading

martin-mao May 8, 2019

benraskin92 May 8, 2019

martin-mao May 8, 2019

martin-mao May 8, 2019

benraskin92 May 8, 2019

benraskin92 May 8, 2019

martin-mao May 8, 2019

mway left a comment

mway May 8, 2019

mway left a comment

robskillington left a comment

arnikola May 9, 2019 •

edited

Loading

benraskin92 May 9, 2019

arnikola May 9, 2019

arnikola May 9, 2019

benraskin92 May 9, 2019

		@@ -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

[query] Fix step parsing #1617

[query] Fix step parsing #1617

Conversation

benraskin92 commented May 8, 2019

codecov bot commented May 8, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mway left a comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

arnikola May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 8, 2019 •

edited

Loading

arnikola May 9, 2019 •

edited

Loading