Fix invalid start/end params in series api #2015

yeya24 · 2020-01-20T20:42:45Z

Signed-off-by: yeya24 yb532204897@gmail.com

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Fixes #2011

Verification

I have already tested both in Grafana and local API request.

curl -g 'http://localhost:10908/api/v1/series?' -d 'match[]={__name__="up"}'
{"status":"success","data":[{"__name__":"up","instance":"localhost:9090","job":"prometheus","replica":"test"},{"__name__":"up","instance":"localhost:9091","job":"prometheus","replica":"test"}]}

curl -g 'http://localhost:10908/api/v1/series?' -d 'match[]={__name__="up"}' -d 'start=1579551916'   
{"status":"success","data":[{"__name__":"up","instance":"localhost:9091","job":"prometheus","replica":"test"}]}

Signed-off-by: yeya24 <yb532204897@gmail.com>

yeya24 · 2020-01-20T20:46:08Z

pkg/store/prometheus.go

@@ -663,9 +666,9 @@ func (p *PrometheusStore) seriesLabels(ctx context.Context, matchers []storepb.L
 	}

 	q.Add("match[]", metric)
+	q.Add("start", formatTime(timestamp.Time(startTime)))


I am not sure if there is a better way to format the timestamp from int64 directly? I am not quite familiar with this

Looks like time.RFC3339Nano is supported so maybe startTime.Format(time.RFC3339Nano)?

I have tried several times but still doesn't work, the generated timestamp is not valid in Prometheus side. Are you OK with the current implementation? @bwplotka
I think there is no big difference between the two ways.

It would be super nice to make it valid, I can't see reason why it would not work:

The code is literally this:

func parseTime(s string) (time.Time, error) { if t, err := strconv.ParseFloat(s, 64); err == nil { s, ns := math.Modf(t) ns = math.Round(ns*1000) / 1000 return time.Unix(int64(s), int64(ns*float64(time.Second))), nil } if t, err := time.Parse(time.RFC3339Nano, s); err == nil { return t, nil } // Stdlib's time parser can only handle 4 digit years. As a workaround until // that is fixed we want to at least support our own boundary times. // Context: https://github.com/prometheus/client_golang/issues/614 // Upstream issue: https://github.com/golang/go/issues/20555 switch s { case minTimeFormatted: return minTime, nil case maxTimeFormatted: return maxTime, nil } return time.Time{}, errors.Errorf("cannot parse %q to a valid timestamp", s) }

so you can test even locally for that.

https://github.com/prometheus/prometheus/blob/9d9bc524e5e893287cfeeab9995252f54b0624a8/web/api/v1/api.go#L1400

Hi, I think that may bring this more complicated. What about just using the current implementation in this PR and maybe add a TODO for changing to RFC3339 format? The current way works, and I think we need to fix #2011 ASAP

Or could you reproduce the error I met in your setup @bwplotka

@yeya24 maybe let's add another test-case which would test this out so that we would know for sure if this is reproducible?

@GiedriusS Added one. From that testcase, using timestamp.Time(startTime).Format(time.RFC3339Nano) will fail. I think it is OK to use the current one.

It is unfortunate that Golang's stdlib cannot parse all of the values that one can format in time.RFC3339Nano: golang/go#20555. However, I'd vote for @bwplotka's suggestion here: maybe lets just not add these parameters if they haven't been specified? Prometheus then handles all of this accordingly. I think we'd avoid more problems down the line. WDYT?

Thanks for pointing this out! However, I am still not sure about how to handle all the cases.

Currently the minTime of sidecar can be configured via a flag. But its default value "0000-01-01T00:00:00Z" is invalid at the Prometheus side. How to avoid this case? And what if users set some other time that cannot be parsed as well? @GiedriusS

bwplotka

Thanks!

One suggestion and LGTM! Thanks for fixing 👍

bwplotka · 2020-01-20T21:24:40Z

pkg/store/prometheus.go

@@ -663,9 +666,9 @@ func (p *PrometheusStore) seriesLabels(ctx context.Context, matchers []storepb.L
 	}

 	q.Add("match[]", metric)
+	q.Add("start", formatTime(timestamp.Time(startTime)))


Looks like time.RFC3339Nano is supported so maybe startTime.Format(time.RFC3339Nano)?

Signed-off-by: yeya24 <yb532204897@gmail.com>

sylr · 2020-01-22T20:24:34Z

I've built this PR and unfortunately it did not fix the problem for me.

Grafana queries that populate variables returned more items that they should have.

yeya24 · 2020-01-22T22:35:43Z

I've built this PR and unfortunately it did not fix the problem for me.

Grafana queries that populate variables returned more items that they should have.

Hi, @sylr
Could you please share with me how you test this?
I am not quite familiar with Grafana and I only found the variables page calls the series API.

Recent 30 min of label_values(up, instance)

Several days ago data of label_values(up, instance). The result is different from the previous one.

bwplotka

After some digging I believe we should merge and release this version. @yeya24 is right, formatting to float actually makes more sense. See: prometheus/client_golang#614

bwplotka · 2020-01-23T07:36:57Z

Thank you @yeya24 for quick reaction!

sylr · 2020-01-23T08:56:04Z

Ok so I'd like to correct my previous comment as I thought the problem was located only on the querier, it was not according to my new observations:

0.10.0 on the sidecar and 0.10.0 on the querier -> NOT OK
0.10.0 on the sidecar and 0.8.1 on the querier -> OK
master+this on the sidecar and 0.10.0 on the querier -> NOT OK
master+this on the sidecar and master+this on the querier -> OK
0.10.0 on the sidecar and master+this on the querier -> NOT OK

* fix invalid start/end params in series api Signed-off-by: yeya24 <yb532204897@gmail.com> * add testcase for serieslabels time range Signed-off-by: yeya24 <yb532204897@gmail.com>

* Fix invalid start/end params in series api (#2015) * fix invalid start/end params in series api Signed-off-by: yeya24 <yb532204897@gmail.com> * add testcase for serieslabels time range Signed-off-by: yeya24 <yb532204897@gmail.com> * Revert "Fix revert metric name regression (#2005)" This reverts commit 7d16852. * Cut release 0.10.1 Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Co-authored-by: Ben Ye <yb532204897@gmail.com>

fix invalid start/end params in series api

7dfddb6

Signed-off-by: yeya24 <yb532204897@gmail.com>

yeya24 commented Jan 20, 2020

View reviewed changes

bwplotka approved these changes Jan 20, 2020

View reviewed changes

FUSAKLA approved these changes Jan 22, 2020

View reviewed changes

s-urbaniak mentioned this pull request Jan 22, 2020

Bump v0.10.0 openshift/thanos#22

Closed

add testcase for serieslabels time range

192a24d

Signed-off-by: yeya24 <yb532204897@gmail.com>

bwplotka approved these changes Jan 23, 2020

View reviewed changes

bwplotka merged commit 43f659a into thanos-io:master Jan 23, 2020

yeya24 deleted the fix-series-timerange branch January 23, 2020 12:57

gjtempleton mentioned this pull request Feb 4, 2020

[prometheus-thanos] - Update to v0.10.1 kiwigrid/helm-charts#288

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid start/end params in series api #2015

Fix invalid start/end params in series api #2015

yeya24 commented Jan 20, 2020 •

edited

Loading

yeya24 Jan 20, 2020

bwplotka Jan 20, 2020

yeya24 Jan 20, 2020

bwplotka Jan 21, 2020

bwplotka Jan 21, 2020

yeya24 Jan 22, 2020 •

edited

Loading

GiedriusS Jan 22, 2020

yeya24 Jan 22, 2020

GiedriusS Jan 22, 2020

yeya24 Jan 22, 2020 •

edited

Loading

bwplotka left a comment

bwplotka Jan 20, 2020

sylr commented Jan 22, 2020

yeya24 commented Jan 22, 2020

bwplotka left a comment

bwplotka commented Jan 23, 2020

sylr commented Jan 23, 2020

Fix invalid start/end params in series api #2015

Fix invalid start/end params in series api #2015

Conversation

yeya24 commented Jan 20, 2020 • edited Loading

Changes

Verification

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

yeya24 Jan 22, 2020 • 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

yeya24 Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylr commented Jan 22, 2020

yeya24 commented Jan 22, 2020

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Jan 23, 2020

sylr commented Jan 23, 2020

yeya24 commented Jan 20, 2020 •

edited

Loading

yeya24 Jan 22, 2020 •

edited

Loading

yeya24 Jan 22, 2020 •

edited

Loading