Skip to content

Fixed query start/end timestamp rounding #2990

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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## master / unreleased

* [ENHANCEMENT] Query-tee: added a small tolerance to floating point sample values comparison. #2994
* [BUGFIX] Query-frontend: Fixed rounding for incoming query timestamps, to be 100% Prometheus compatible. #2990

## 1.3.0 in progress

Expand Down
12 changes: 11 additions & 1 deletion integration/e2ecortex/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,22 @@ func (c *Client) Push(timeseries []prompb.TimeSeries) (*http.Response, error) {
return res, nil
}

// Query runs a query
// Query runs an instant query.
func (c *Client) Query(query string, ts time.Time) (model.Value, error) {
value, _, err := c.querierClient.Query(context.Background(), query, ts)
return value, err
}

// Query runs a query range.
func (c *Client) QueryRange(query string, start, end time.Time, step time.Duration) (model.Value, error) {
value, _, err := c.querierClient.QueryRange(context.Background(), query, promv1.Range{
Start: start,
End: end,
Step: step,
})
return value, err
}

func (c *Client) QueryRaw(query string) (*http.Response, []byte, error) {
addr := fmt.Sprintf("http://%s/api/prom/api/v1/query?query=%s", c.querierAddress, url.QueryEscape(query))

Expand Down
22 changes: 19 additions & 3 deletions integration/query_frontend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,30 @@ func runQueryFrontendTest(t *testing.T, testMissingMetricName bool, setup queryF
c, err := e2ecortex.NewClient("", queryFrontend.HTTPEndpoint(), "", "", fmt.Sprintf("user-%d", userID))
require.NoError(t, err)

// No need to repeat this test for each user.
// No need to repeat the test on missing metric name for each user.
if userID == 0 && testMissingMetricName {
res, body, err := c.QueryRaw("{instance=~\"hello.*\"}")
require.NoError(t, err)
require.Equal(t, 422, res.StatusCode)
require.Contains(t, string(body), "query must contain metric name")
}

// No need to repeat the test on start/end time rounding for each user.
if userID == 0 {
start := time.Unix(1595846748, 806*1e6)
end := time.Unix(1595846750, 806*1e6)

result, err := c.QueryRange("time()", start, end, time.Second)
require.NoError(t, err)
require.Equal(t, model.ValMatrix, result.Type())

matrix := result.(model.Matrix)
require.Len(t, matrix, 1)
require.Len(t, matrix[0].Values, 3)
assert.Equal(t, model.Time(1595846748806), matrix[0].Values[0].Timestamp)
assert.Equal(t, model.Time(1595846750806), matrix[0].Values[2].Timestamp)
}

for q := 0; q < numQueriesPerUser; q++ {
go func() {
defer wg.Done()
Expand All @@ -197,9 +213,9 @@ func runQueryFrontendTest(t *testing.T, testMissingMetricName bool, setup queryF

wg.Wait()

extra := float64(0)
extra := float64(1)
if testMissingMetricName {
extra = 1
extra++
}
require.NoError(t, queryFrontend.WaitSumMetrics(e2e.Equals(numUsers*numQueriesPerUser+extra), "cortex_query_frontend_queries_total"))

Expand Down
1 change: 1 addition & 0 deletions pkg/util/time.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ func TimeFromMillis(ms int64) time.Time {
func ParseTime(s string) (int64, error) {
if t, err := strconv.ParseFloat(s, 64); err == nil {
s, ns := math.Modf(t)
ns = math.Round(ns*1000) / 1000
tm := time.Unix(int64(s), int64(ns*float64(time.Second)))
return TimeToMillis(tm), nil
}
Expand Down
50 changes: 50 additions & 0 deletions pkg/util/time_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,53 @@ func TestDurationWithJitter(t *testing.T) {
func TestDurationWithJitter_ZeroInputDuration(t *testing.T) {
assert.Equal(t, time.Duration(0), DurationWithJitter(time.Duration(0), 0.5))
}

func TestParseTime(t *testing.T) {
var tests = []struct {
input string
fail bool
result time.Time
}{
{
input: "",
fail: true,
}, {
input: "abc",
fail: true,
}, {
input: "30s",
fail: true,
}, {
input: "123",
result: time.Unix(123, 0),
}, {
input: "123.123",
result: time.Unix(123, 123000000),
}, {
input: "2015-06-03T13:21:58.555Z",
result: time.Unix(1433337718, 555*time.Millisecond.Nanoseconds()),
}, {
input: "2015-06-03T14:21:58.555+01:00",
result: time.Unix(1433337718, 555*time.Millisecond.Nanoseconds()),
}, {
// Test nanosecond rounding.
input: "2015-06-03T13:21:58.56789Z",
result: time.Unix(1433337718, 567*1e6),
}, {
// Test float rounding.
input: "1543578564.705",
result: time.Unix(1543578564, 705*1e6),
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about `input: "2015-06-03T13:21:58.56789Z" ... should that be rounded on milliseconds precision?

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 copied the exact implementation and tests of Prometheus. Should we keep it 1-1 with Prometheus or change it? I'm open to discuss it.

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion on milliseconds rounding... We can keep it as-is to be consistent with Prometheus, and think about it when (if) someone hits that. Then we can clarify at both places (depending on what Prometheus decides)

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've added a test. I believe we should just use the Prometheus implementation as reference implementation (we want to be 100% compatible with Prometheus) and thus I've written the expected value with the current Prometheus rounding.


for _, test := range tests {
ts, err := ParseTime(test.input)
if test.fail {
require.Error(t, err)
continue
}

require.NoError(t, err)
assert.Equal(t, TimeToMillis(test.result), ts)
}
}