From 656a2ac8db9190984a4fe1526c60c9e86ec23efd Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Tue, 24 Mar 2020 14:21:52 -0400 Subject: [PATCH 1/5] [query] Add ability to set default query timeout by config --- src/cmd/services/m3query/config/config.go | 18 ++++++++++++++++++ src/query/api/v1/options/handler.go | 20 ++++++++------------ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/cmd/services/m3query/config/config.go b/src/cmd/services/m3query/config/config.go index 00001fbd89..8e7769bef9 100644 --- a/src/cmd/services/m3query/config/config.go +++ b/src/cmd/services/m3query/config/config.go @@ -57,6 +57,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.Duration ) var ( @@ -124,6 +126,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"` @@ -190,6 +195,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/options/handler.go b/src/query/api/v1/options/handler.go index 85c8fc6fa6..da91d5e51b 100644 --- a/src/query/api/v1/options/handler.go +++ b/src/query/api/v1/options/handler.go @@ -190,17 +190,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 +207,6 @@ func NewHandlerOptions( embeddedDbCfg: embeddedDbCfg, createdAt: time.Now(), tagOptions: tagOptions, - timeoutOpts: timeoutOpts, enforcer: enforcer, fetchOptionsBuilder: fetchOptionsBuilder, queryContextOptions: queryContextOptions, @@ -222,6 +215,9 @@ func NewHandlerOptions( placementServiceNames: placementServiceNames, serviceOptionDefaults: serviceOptionDefaults, nowFn: time.Now, + timeoutOpts: &prometheus.TimeoutOpts{ + FetchTimeout: timeout, + }, }, nil } From a595d5919b72ec2f54a038df94d5e0cf4b437467 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Tue, 24 Mar 2020 14:33:55 -0400 Subject: [PATCH 2/5] Add request param parsing for timeout --- src/query/api/v1/handler/prometheus/common.go | 8 +++- .../api/v1/handler/prometheus/common_test.go | 44 ++++++++++++++++--- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/query/api/v1/handler/prometheus/common.go b/src/query/api/v1/handler/prometheus/common.go index f385cc01e3..bd55fc42ee 100644 --- a/src/query/api/v1/handler/prometheus/common.go +++ b/src/query/api/v1/handler/prometheus/common.go @@ -110,7 +110,13 @@ func ParseRequestTimeout( r *http.Request, configFetchTimeout time.Duration, ) (time.Duration, error) { - timeout := r.Header.Get("timeout") + var timeout string + if v := r.Header.Get("timeout"); v != "" { + timeout = v + } + if v := r.FormValue("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..f9eaf967a1 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 } From 7d374e7330c588dea0f7c02012b51133f3cf5928 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Tue, 24 Mar 2020 14:35:15 -0400 Subject: [PATCH 3/5] Fix build --- src/cmd/services/m3query/config/config.go | 2 +- src/query/api/v1/options/handler.go | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/cmd/services/m3query/config/config.go b/src/cmd/services/m3query/config/config.go index 8e7769bef9..c055944b44 100644 --- a/src/cmd/services/m3query/config/config.go +++ b/src/cmd/services/m3query/config/config.go @@ -58,7 +58,7 @@ const ( "generation scheme is required in coordinator configuration settings. " + "More information is available here: %s" - defaultQueryTimeout = 30 * time.Duration + defaultQueryTimeout = 30 * time.Second ) var ( diff --git a/src/query/api/v1/options/handler.go b/src/query/api/v1/options/handler.go index da91d5e51b..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" @@ -191,9 +190,9 @@ func NewHandlerOptions( serviceOptionDefaults []handleroptions.ServiceOptionsDefault, ) (HandlerOptions, error) { timeout := cfg.Query.TimeoutOrDefault() - if embeddedDbCfg != nil - && embeddedDbCfg.Client.FetchTimeout != nil - && *embeddedDbCfg.Client.FetchTimeout > timeout { + if embeddedDbCfg != nil && + embeddedDbCfg.Client.FetchTimeout != nil && + *embeddedDbCfg.Client.FetchTimeout > timeout { timeout = *embeddedDbCfg.Client.FetchTimeout } @@ -215,7 +214,7 @@ func NewHandlerOptions( placementServiceNames: placementServiceNames, serviceOptionDefaults: serviceOptionDefaults, nowFn: time.Now, - timeoutOpts: &prometheus.TimeoutOpts{ + timeoutOpts: &prometheus.TimeoutOpts{ FetchTimeout: timeout, }, }, nil From 04fede902ef456a9a104e57d2cd89c74a62bba4c Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Tue, 24 Mar 2020 15:08:45 -0400 Subject: [PATCH 4/5] Fix test --- .../api/v1/handler/prometheus/common_test.go | 2 +- src/query/api/v1/httpd/handler_test.go | 28 ------------------- 2 files changed, 1 insertion(+), 29 deletions(-) diff --git a/src/query/api/v1/handler/prometheus/common_test.go b/src/query/api/v1/handler/prometheus/common_test.go index f9eaf967a1..7d84bdb938 100644 --- a/src/query/api/v1/handler/prometheus/common_test.go +++ b/src/query/api/v1/handler/prometheus/common_test.go @@ -39,7 +39,7 @@ import ( ) func TestPromCompressedReadSuccess(t *testing.T) { - req := httptest.NewRequest("POST", "dummy", test.GeneratePromReadBody(t)) + req := httptest.NewRequest("POST", "/dummy", test.GeneratePromReadBody(t)) _, err := ParsePromCompressedRequest(req) assert.NoError(t, err) } 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) From aec0ed351af3fd2cfe71a7cad12987913366ec5a Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 9 Apr 2020 10:46:59 -0400 Subject: [PATCH 5/5] Update common.go --- src/query/api/v1/handler/prometheus/common.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/query/api/v1/handler/prometheus/common.go b/src/query/api/v1/handler/prometheus/common.go index bd55fc42ee..e12357bf9e 100644 --- a/src/query/api/v1/handler/prometheus/common.go +++ b/src/query/api/v1/handler/prometheus/common.go @@ -111,12 +111,14 @@ func ParseRequestTimeout( configFetchTimeout time.Duration, ) (time.Duration, error) { var timeout string - if v := r.Header.Get("timeout"); v != "" { + if v := r.FormValue("timeout"); v != "" { timeout = v } - if v := r.FormValue("timeout"); v != "" { + // Note: Header should take precedence. + if v := r.Header.Get("timeout"); v != "" { timeout = v } + if timeout == "" { return configFetchTimeout, nil }