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

Jump to bottom

Fix invalid start/end params in series api #2015

Merged
merged 2 commits into from
Jan 23, 2020
Merged

Fix invalid start/end params in series api #2015

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7dfddb6
fix invalid start/end params in series api
yeya24 Jan 20, 2020
192a24d
add testcase for serieslabels time range
yeya24 Jan 22, 2020
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
2 changes: 1 addition & 1 deletion pkg/model/timeduration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func (tdv *TimeOrDurationValue) Set(s string) error {
return nil
}

// String returns either tume or duration.
// String returns either time or duration.
func (tdv *TimeOrDurationValue) String() string {
switch {
case tdv.Time != nil:
Expand Down
11 changes: 9 additions & 2 deletions pkg/store/prometheus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ import (
"net/url"
"path"
"sort"
"strconv"
"strings"
"sync"
"time"

"github.com/go-kit/kit/log"
"github.com/go-kit/kit/log/level"
Expand All @@ -23,6 +25,7 @@ import (
"github.com/pkg/errors"
"github.com/prometheus/common/version"
"github.com/prometheus/prometheus/pkg/labels"
"github.com/prometheus/prometheus/pkg/timestamp"
"github.com/prometheus/prometheus/prompb"
"github.com/prometheus/prometheus/storage/remote"
"github.com/prometheus/prometheus/tsdb/chunkenc"
Expand Down Expand Up @@ -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)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@yeya24 yeya24 Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@yeya24 yeya24 Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

q.Add("end", formatTime(timestamp.Time(endTime)))
u.RawQuery = q.Encode()
q.Add("start", string(startTime))
q.Add("end", string(endTime))

req, err := http.NewRequest("GET", u.String(), nil)
if err != nil {
Expand Down Expand Up @@ -715,3 +718,7 @@ func (p *PrometheusStore) seriesLabels(ctx context.Context, matchers []storepb.L

return m.Data, nil
}

func formatTime(t time.Time) string {
return strconv.FormatFloat(float64(t.Unix())+float64(t.Nanosecond())/1e9, 'f', -1, 64)
}