From a5a84d0973560801d8a5c7fc5efd594ae9ca533b Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 9 Apr 2020 11:36:20 -0400 Subject: [PATCH] [query] Add ability to set default query timeout by config (#2226) --- src/cmd/services/m3query/config/config.go | 18 ++++++++ src/query/api/v1/handler/prometheus/common.go | 10 ++++- .../api/v1/handler/prometheus/common_test.go | 44 ++++++++++++++++--- src/query/api/v1/httpd/handler_test.go | 28 ------------ src/query/api/v1/options/handler.go | 21 ++++----- 5 files changed, 73 insertions(+), 48 deletions(-) diff --git a/src/cmd/services/m3query/config/config.go b/src/cmd/services/m3query/config/config.go index f795cd5880..303687a8b4 100644 --- a/src/cmd/services/m3query/config/config.go +++ b/src/cmd/services/m3query/config/config.go @@ -62,6 +62,8 @@ const ( errNoIDGenerationScheme = "error: a recent breaking change means that an ID " + "generation scheme is required in coordinator configuration settings. " + "More information is available here: %s" + + defaultQueryTimeout = 30 * time.Second ) var ( @@ -129,6 +131,9 @@ type Configuration struct { // Carbon is the carbon configuration. Carbon *CarbonConfiguration `yaml:"carbon"` + // Query is the query configuration. + Query QueryConfiguration `yaml:"query"` + // Limits specifies limits on per-query resource usage. Limits LimitsConfiguration `yaml:"limits"` @@ -195,6 +200,19 @@ type ResultOptions struct { KeepNans bool `yaml:"keepNans"` } +// QueryConfiguration is the query configuration. +type QueryConfiguration struct { + Timeout *time.Duration `yaml:"timeout"` +} + +// TimeoutOrDefault returns the configured timeout or default value. +func (c QueryConfiguration) TimeoutOrDefault() time.Duration { + if v := c.Timeout; v != nil { + return *v + } + return defaultQueryTimeout +} + // LimitsConfiguration represents limitations on resource usage in the query // instance. Limits are split between per-query and global limits. type LimitsConfiguration struct { diff --git a/src/query/api/v1/handler/prometheus/common.go b/src/query/api/v1/handler/prometheus/common.go index f385cc01e3..e12357bf9e 100644 --- a/src/query/api/v1/handler/prometheus/common.go +++ b/src/query/api/v1/handler/prometheus/common.go @@ -110,7 +110,15 @@ func ParseRequestTimeout( r *http.Request, configFetchTimeout time.Duration, ) (time.Duration, error) { - timeout := r.Header.Get("timeout") + var timeout string + if v := r.FormValue("timeout"); v != "" { + timeout = v + } + // Note: Header should take precedence. + if v := r.Header.Get("timeout"); v != "" { + timeout = v + } + if timeout == "" { return configFetchTimeout, nil } diff --git a/src/query/api/v1/handler/prometheus/common_test.go b/src/query/api/v1/handler/prometheus/common_test.go index dc4ec68ff0..7d84bdb938 100644 --- a/src/query/api/v1/handler/prometheus/common_test.go +++ b/src/query/api/v1/handler/prometheus/common_test.go @@ -23,7 +23,10 @@ package prometheus import ( "bytes" "fmt" + "mime/multipart" "net/http" + "net/http/httptest" + "net/url" "strings" "testing" "time" @@ -32,37 +35,38 @@ import ( "github.com/m3db/m3/src/query/test" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestPromCompressedReadSuccess(t *testing.T) { - req, _ := http.NewRequest("POST", "dummy", test.GeneratePromReadBody(t)) + req := httptest.NewRequest("POST", "/dummy", test.GeneratePromReadBody(t)) _, err := ParsePromCompressedRequest(req) assert.NoError(t, err) } func TestPromCompressedReadNoBody(t *testing.T) { - req, _ := http.NewRequest("POST", "dummy", nil) + req := httptest.NewRequest("POST", "/dummy", nil) _, err := ParsePromCompressedRequest(req) assert.Error(t, err) assert.Equal(t, err.Code(), http.StatusBadRequest) } func TestPromCompressedReadEmptyBody(t *testing.T) { - req, _ := http.NewRequest("POST", "dummy", bytes.NewReader([]byte{})) + req := httptest.NewRequest("POST", "/dummy", bytes.NewReader([]byte{})) _, err := ParsePromCompressedRequest(req) assert.Error(t, err) assert.Equal(t, err.Code(), http.StatusBadRequest) } func TestPromCompressedReadInvalidEncoding(t *testing.T) { - req, _ := http.NewRequest("POST", "dummy", bytes.NewReader([]byte{'a'})) + req := httptest.NewRequest("POST", "/dummy", bytes.NewReader([]byte{'a'})) _, err := ParsePromCompressedRequest(req) assert.Error(t, err) assert.Equal(t, err.Code(), http.StatusBadRequest) } -func TestTimeoutParse(t *testing.T) { - req, _ := http.NewRequest("POST", "dummy", nil) +func TestTimeoutParseWithHeader(t *testing.T) { + req := httptest.NewRequest("POST", "/dummy", nil) req.Header.Add("timeout", "1ms") timeout, err := ParseRequestTimeout(req, time.Second) @@ -79,6 +83,34 @@ func TestTimeoutParse(t *testing.T) { assert.Error(t, err) } +func TestTimeoutParseWithPostRequestParam(t *testing.T) { + params := url.Values{} + params.Add("timeout", "1ms") + + buff := bytes.NewBuffer(nil) + form := multipart.NewWriter(buff) + form.WriteField("timeout", "1ms") + require.NoError(t, form.Close()) + + req := httptest.NewRequest("POST", "/dummy", buff) + req.Header.Set("Content-Type", form.FormDataContentType()) + + timeout, err := ParseRequestTimeout(req, time.Second) + assert.NoError(t, err) + assert.Equal(t, timeout, time.Millisecond) +} + +func TestTimeoutParseWithGetRequestParam(t *testing.T) { + params := url.Values{} + params.Add("timeout", "1ms") + + req := httptest.NewRequest("GET", "/dummy?"+params.Encode(), nil) + + timeout, err := ParseRequestTimeout(req, time.Second) + assert.NoError(t, err) + assert.Equal(t, timeout, time.Millisecond) +} + type writer struct { value string } diff --git a/src/query/api/v1/httpd/handler_test.go b/src/query/api/v1/httpd/handler_test.go index d71d4cfbed..afa733b54f 100644 --- a/src/query/api/v1/httpd/handler_test.go +++ b/src/query/api/v1/httpd/handler_test.go @@ -115,34 +115,6 @@ func setupHandler( return NewHandler(opts, customHandlers...), nil } -func TestHandlerFetchTimeoutError(t *testing.T) { - ctrl := gomock.NewController(t) - storage, _ := m3.NewStorageAndSession(t, ctrl) - downsamplerAndWriter := ingest.NewDownsamplerAndWriter(storage, nil, testWorkerPool) - - negValue := -1 * time.Second - dbconfig := &dbconfig.DBConfiguration{Client: client.Configuration{FetchTimeout: &negValue}} - engine := newEngine(storage, time.Minute, nil, instrument.NewOptions()) - cfg := config.Configuration{LookbackDuration: &defaultLookbackDuration} - _, err := options.NewHandlerOptions( - downsamplerAndWriter, - makeTagOptions(), - engine, - nil, - nil, - cfg, - dbconfig, - nil, - handleroptions.NewFetchOptionsBuilder(handleroptions.FetchOptionsBuilderOptions{}), - models.QueryContextOptions{}, - instrument.NewOptions(), - defaultCPUProfileduration, - defaultPlacementServices, - svcDefaultOptions) - - require.Error(t, err) -} - func TestHandlerFetchTimeout(t *testing.T) { ctrl := gomock.NewController(t) storage, _ := m3.NewStorageAndSession(t, ctrl) diff --git a/src/query/api/v1/options/handler.go b/src/query/api/v1/options/handler.go index 85c8fc6fa6..40f6450481 100644 --- a/src/query/api/v1/options/handler.go +++ b/src/query/api/v1/options/handler.go @@ -21,7 +21,6 @@ package options import ( - "fmt" "net/http" "time" @@ -190,17 +189,11 @@ func NewHandlerOptions( placementServiceNames []string, serviceOptionDefaults []handleroptions.ServiceOptionsDefault, ) (HandlerOptions, error) { - var timeoutOpts = &prometheus.TimeoutOpts{} - if embeddedDbCfg == nil || embeddedDbCfg.Client.FetchTimeout == nil { - timeoutOpts.FetchTimeout = defaultTimeout - } else { - timeout := *embeddedDbCfg.Client.FetchTimeout - if timeout <= 0 { - return nil, - fmt.Errorf("m3db client fetch timeout should be > 0, is %d", timeout) - } - - timeoutOpts.FetchTimeout = timeout + timeout := cfg.Query.TimeoutOrDefault() + if embeddedDbCfg != nil && + embeddedDbCfg.Client.FetchTimeout != nil && + *embeddedDbCfg.Client.FetchTimeout > timeout { + timeout = *embeddedDbCfg.Client.FetchTimeout } return &handlerOptions{ @@ -213,7 +206,6 @@ func NewHandlerOptions( embeddedDbCfg: embeddedDbCfg, createdAt: time.Now(), tagOptions: tagOptions, - timeoutOpts: timeoutOpts, enforcer: enforcer, fetchOptionsBuilder: fetchOptionsBuilder, queryContextOptions: queryContextOptions, @@ -222,6 +214,9 @@ func NewHandlerOptions( placementServiceNames: placementServiceNames, serviceOptionDefaults: serviceOptionDefaults, nowFn: time.Now, + timeoutOpts: &prometheus.TimeoutOpts{ + FetchTimeout: timeout, + }, }, nil }