Skip to content

Commit

Permalink
Add default "time" param to parsed form (if any) (grafana#3599)
Browse files Browse the repository at this point in the history 
* Add default "time" param to parsed form (if any)

If the form was already parsed by a downstream middleware, we need to
add it to that form too, otherwise it won't be available to upstream by
just adding it to the url query.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Check that form doesn't have "time"

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
  • Loading branch information
@colega
colega authored and mason committed Dec 16, 2022
1 parent 2e558a9 commit c48b37a
Show file tree
Hide file tree
Showing 3 changed files with 137 additions and 18 deletions.
15 changes: 11 additions & 4 deletions pkg/frontend/querymiddleware/roundtrip.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,6 @@ func newQueryTripperware(
queryrange := newLimitedParallelismRoundTripper(next, codec, limits, queryRangeMiddleware...)
instant := defaultInstantQueryParamsRoundTripper(
newLimitedParallelismRoundTripper(next, codec, limits, queryInstantMiddleware...),
time.Now,
)
return RoundTripFunc(func(r *http.Request) (*http.Response, error) {
switch {
Expand Down Expand Up @@ -308,12 +307,20 @@ func isInstantQuery(path string) bool {
return strings.HasSuffix(path, instantQueryPathSuffix)
}

func defaultInstantQueryParamsRoundTripper(next http.RoundTripper, now func() time.Time) http.RoundTripper {
func defaultInstantQueryParamsRoundTripper(next http.RoundTripper) http.RoundTripper {
return RoundTripFunc(func(r *http.Request) (*http.Response, error) {
if isInstantQuery(r.URL.Path) && !r.URL.Query().Has("time") {
if isInstantQuery(r.URL.Path) && !r.Form.Has("time") && !r.URL.Query().Has("time") {
nowUnixStr := strconv.FormatInt(time.Now().Unix(), 10)

q := r.URL.Query()
q.Add("time", strconv.FormatInt(time.Now().Unix(), 10))
q.Add("time", nowUnixStr)
r.URL.RawQuery = q.Encode()

// If form was already parsed, add this param to the form too.
// (The form doesn't have "time", otherwise we'd not be here)
if r.Form != nil {
r.Form.Set("time", nowUnixStr)
}
}
return next.RoundTrip(r)
})
Expand Down
81 changes: 74 additions & 7 deletions pkg/frontend/querymiddleware/roundtrip_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ func TestInstantTripperware(t *testing.T) {
)
require.NoError(t, err)

ts := time.Date(2021, 1, 2, 3, 4, 5, 0, time.UTC)
rt := RoundTripFunc(func(r *http.Request) (*http.Response, error) {
// We will provide a sample exactly for the requested time,
// this way we'll also be able to tell which time was requested.
Expand All @@ -153,27 +152,95 @@ func TestInstantTripperware(t *testing.T) {
})
})

queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: tw(rt)})
require.NoError(t, err)
api := v1.NewAPI(queryClient)
tripper := tw(rt)

t.Run("specific time happy case", func(t *testing.T) {
queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: tripper})
require.NoError(t, err)
api := v1.NewAPI(queryClient)

t.Run("happy case roundtrip", func(t *testing.T) {
ts := time.Date(2021, 1, 2, 3, 4, 5, 0, time.UTC)
res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, ts)
require.NoError(t, err)
require.Equal(t, model.Vector{
{Metric: model.Metric{"foo": "bar"}, Timestamp: model.TimeFromUnixNano(ts.UnixNano()), Value: totalShards},
}, res)
})

t.Run("default time param", func(t *testing.T) {
res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, time.Now())
t.Run("specific time param with form being already parsed", func(t *testing.T) {
ts := time.Date(2021, 1, 2, 3, 4, 5, 0, time.UTC)

formParserRoundTripper := RoundTripFunc(func(r *http.Request) (*http.Response, error) {
assert.NoError(t, r.ParseForm())
return tripper.RoundTrip(r)
})
queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: formParserRoundTripper})
require.NoError(t, err)
api := v1.NewAPI(queryClient)

res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, ts)
require.NoError(t, err)
require.IsType(t, model.Vector{}, res)
require.NotEmpty(t, res.(model.Vector))

resultTime := res.(model.Vector)[0].Timestamp.Time()
require.Equal(t, ts.Unix(), resultTime.Unix())
})

t.Run("default time param happy case", func(t *testing.T) {
queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: tripper})
require.NoError(t, err)
api := v1.NewAPI(queryClient)

res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, time.Time{})
require.NoError(t, err)
require.IsType(t, model.Vector{}, res)
require.NotEmpty(t, res.(model.Vector))

resultTime := res.(model.Vector)[0].Timestamp.Time()
require.InDelta(t, time.Now().Unix(), resultTime.Unix(), 1)
})

t.Run("default time param with form being already parsed", func(t *testing.T) {
formParserRoundTripper := RoundTripFunc(func(r *http.Request) (*http.Response, error) {
assert.NoError(t, r.ParseForm())
return tripper.RoundTrip(r)
})
queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: formParserRoundTripper})
require.NoError(t, err)
api := v1.NewAPI(queryClient)

res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, time.Time{})
require.NoError(t, err)
require.IsType(t, model.Vector{}, res)
require.NotEmpty(t, res.(model.Vector))

resultTime := res.(model.Vector)[0].Timestamp.Time()
require.InDelta(t, time.Now().Unix(), resultTime.Unix(), 1)
})

t.Run("post form time param takes precedence over query time param ", func(t *testing.T) {
postFormTimeParam := time.Date(2021, 1, 2, 3, 4, 5, 0, time.UTC)

addQueryTimeParam := RoundTripFunc(func(r *http.Request) (*http.Response, error) {
query := r.URL.Query()
// Set query's "time" param to something wrong, so that it would fail to be parsed if it's used.
query.Set("time", "query-time-param-should-not-be-used")
r.URL.RawQuery = query.Encode()
return tripper.RoundTrip(r)
})
queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: addQueryTimeParam})
require.NoError(t, err)
api := v1.NewAPI(queryClient)

res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, postFormTimeParam)
require.NoError(t, err)
require.IsType(t, model.Vector{}, res)
require.NotEmpty(t, res.(model.Vector))

resultTime := res.(model.Vector)[0].Timestamp.Time()
require.Equal(t, postFormTimeParam.Unix(), resultTime.Unix())
})
}

func TestTripperware_Metrics(t *testing.T) {
Expand Down
59 changes: 52 additions & 7 deletions pkg/frontend/transport/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"io"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"

Expand Down Expand Up @@ -52,21 +53,67 @@ func TestHandler_ServeHTTP(t *testing.T) {
for _, tt := range []struct {
name string
cfg HandlerConfig
request func() *http.Request
expectedParams url.Values
expectedMetrics int
}{
{
name: "test handler with stats enabled",
cfg: HandlerConfig{QueryStatsEnabled: true},
name: "handler with stats enabled, POST request with params",
cfg: HandlerConfig{QueryStatsEnabled: true, MaxBodySize: 1024},
request: func() *http.Request {
form := url.Values{
"query": []string{"some_metric"},
"time": []string{"42"},
}
r := httptest.NewRequest("POST", "/api/v1/query", strings.NewReader(form.Encode()))
r.Header.Add("Content-Type", "application/x-www-form-urlencoded")
return r
},
expectedParams: url.Values{
"query": []string{"some_metric"},
"time": []string{"42"},
},
expectedMetrics: 4,
},
{
name: "test handler with stats disabled",
cfg: HandlerConfig{QueryStatsEnabled: false},
name: "handler with stats enabled, GET request with params",
cfg: HandlerConfig{QueryStatsEnabled: true},
request: func() *http.Request {
return httptest.NewRequest("GET", "/api/v1/query?query=some_metric&time=42", nil)
},
expectedParams: url.Values{
"query": []string{"some_metric"},
"time": []string{"42"},
},
expectedMetrics: 4,
},
{
name: "handler with stats enabled, GET request without params",
cfg: HandlerConfig{QueryStatsEnabled: true},
request: func() *http.Request {
return httptest.NewRequest("GET", "/api/v1/query", nil)
},
expectedParams: url.Values{},
expectedMetrics: 4,
},
{
name: "handler with stats disabled, GET request with params",
cfg: HandlerConfig{QueryStatsEnabled: false},
request: func() *http.Request {
return httptest.NewRequest("GET", "/api/v1/query?query=some_metric&time=42", nil)
},
expectedParams: url.Values{
"query": []string{"some_metric"},
"time": []string{"42"},
},
expectedMetrics: 0,
},
} {
t.Run(tt.name, func(t *testing.T) {
roundTripper := roundTripperFunc(func(req *http.Request) (*http.Response, error) {
assert.NoError(t, req.ParseForm())
assert.Equal(t, tt.expectedParams, req.Form)

return &http.Response{
StatusCode: http.StatusOK,
Body: io.NopCloser(strings.NewReader("{}")),
Expand All @@ -76,9 +123,7 @@ func TestHandler_ServeHTTP(t *testing.T) {
reg := prometheus.NewPedanticRegistry()
handler := NewHandler(tt.cfg, roundTripper, log.NewNopLogger(), reg)

ctx := user.InjectOrgID(context.Background(), "12345")
req := httptest.NewRequest("GET", "/", nil)
req = req.WithContext(ctx)
req := tt.request().WithContext(user.InjectOrgID(context.Background(), "12345"))
resp := httptest.NewRecorder()

handler.ServeHTTP(resp, req)
Expand Down

0 comments on commit c48b37a

Please sign in to comment.