[query] Add ability to set default query timeout by config (#2226)

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 {

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
 	}

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
 }

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)

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
 }