From 2f48fb52f13aafcf9e6471e7a662de1cc664b48b Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Tue, 7 May 2019 13:31:59 -0400 Subject: [PATCH 1/2] [query] Fix nil panic for error response for PromReadInstantHandler --- .../prometheus/native/read_instantaneous.go | 4 +- .../native/read_instantaneous_test.go | 172 ++++++++++++++++++ .../v1/handler/prometheus/native/read_test.go | 45 +++-- 3 files changed, 202 insertions(+), 19 deletions(-) create mode 100644 src/query/api/v1/handler/prometheus/native/read_instantaneous_test.go diff --git a/src/query/api/v1/handler/prometheus/native/read_instantaneous.go b/src/query/api/v1/handler/prometheus/native/read_instantaneous.go index 18a368a93c..e5bcdeb6bf 100644 --- a/src/query/api/v1/handler/prometheus/native/read_instantaneous.go +++ b/src/query/api/v1/handler/prometheus/native/read_instantaneous.go @@ -74,13 +74,13 @@ func (h *PromReadInstantHandler) ServeHTTP(w http.ResponseWriter, r *http.Reques } if params.Debug { - logger.Info("Request params", zap.Any("params", params)) + logger.Info("request params", zap.Any("params", params)) } result, err := read(ctx, h.engine, h.tagOpts, w, params) if err != nil { logger.Error("unable to fetch data", zap.Error(err)) - httperrors.ErrorWithReqInfo(w, r, http.StatusBadRequest, rErr) + httperrors.ErrorWithReqInfo(w, r, http.StatusInternalServerError, err) return } diff --git a/src/query/api/v1/handler/prometheus/native/read_instantaneous_test.go b/src/query/api/v1/handler/prometheus/native/read_instantaneous_test.go new file mode 100644 index 0000000000..52f9a8ee92 --- /dev/null +++ b/src/query/api/v1/handler/prometheus/native/read_instantaneous_test.go @@ -0,0 +1,172 @@ +// Copyright (c) 2019 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package native + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "testing" + "time" + + "github.com/m3db/m3/src/query/block" + "github.com/m3db/m3/src/query/test" + "github.com/m3db/m3/src/query/util/logging" + xtest "github.com/m3db/m3/src/x/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type vectorResult struct { + Data struct { + Result []struct { + Metric map[string]string `json:"metric"` + Value vectorResultValues `json:"value"` + } `json:"result"` + } `json:"data"` +} + +type vectorResultValues []interface{} + +func (v vectorResultValues) parse() (time.Time, int, error) { + if len(v) != 2 { + return time.Time{}, 0, + fmt.Errorf("expected length 2: actual=%d", len(v)) + } + + t, ok := v[0].(float64) + if !ok { + return time.Time{}, 0, + fmt.Errorf("could not unmarshal time: %v", v[0]) + } + + str, ok := v[1].(string) + if !ok { + return time.Time{}, 0, + fmt.Errorf("could not unmarshal value: %v", v[1]) + } + + n, err := strconv.Atoi(str) + if err != nil { + return time.Time{}, 0, + fmt.Errorf("could not convert value to number: err=%v", err) + } + + return time.Unix(int64(t), 0), n, nil +} + +func TestPromReadInstantHandler(t *testing.T) { + logging.InitWithCores(nil) + + values, bounds := test.GenerateValuesAndBounds(nil, nil) + + setup := newTestSetup() + promReadInstant := setup.Handlers.InstantRead + + b := test.NewBlockFromValues(bounds, values) + setup.Storage.SetFetchBlocksResult(block.Result{Blocks: []block.Block{b}}, nil) + + req := httptest.NewRequest(PromReadInstantHTTPMethod, PromReadInstantURL, nil) + + params := url.Values{} + params.Set(queryParam, "dummy0{}") + + req.URL.RawQuery = params.Encode() + + recorder := httptest.NewRecorder() + promReadInstant.ServeHTTP(recorder, req) + + require.Equal(t, http.StatusOK, recorder.Result().StatusCode) + + var result vectorResult + require.NoError(t, json.Unmarshal(recorder.Body.Bytes(), &result)) + require.Equal(t, 2, len(result.Data.Result)) + + at0, value0, err := result.Data.Result[0].Value.parse() + require.NoError(t, err) + at1, value1, err := result.Data.Result[1].Value.parse() + require.NoError(t, err) + + expected := mustPrettyJSON(t, fmt.Sprintf(` + { + "status": "success", + "data": { + "resultType": "vector", + "result": [ + { + "metric": { + "__name__": "dummy0", + "dummy0": "dummy0" + }, + "value": [ + %d, + "%d" + ] + }, + { + "metric": { + "__name__": "dummy1", + "dummy1": "dummy1" + }, + "value": [ + %d, + "%d" + ] + } + ] + } + } + `, at0.Unix(), value0, at1.Unix(), value1)) + actual := mustPrettyJSON(t, recorder.Body.String()) + assert.Equal(t, expected, actual, xtest.Diff(expected, actual)) +} + +func TestPromReadInstantHandlerStorageError(t *testing.T) { + logging.InitWithCores(nil) + + setup := newTestSetup() + promReadInstant := setup.Handlers.InstantRead + + storageErr := fmt.Errorf("storage err") + setup.Storage.SetFetchBlocksResult(block.Result{}, storageErr) + + req := httptest.NewRequest(PromReadInstantHTTPMethod, PromReadInstantURL, nil) + + params := url.Values{} + params.Set(queryParam, "dummy0{}") + + req.URL.RawQuery = params.Encode() + + recorder := httptest.NewRecorder() + promReadInstant.ServeHTTP(recorder, req) + + require.Equal(t, http.StatusInternalServerError, recorder.Result().StatusCode) + + var errResp struct { + Error string `json:"error"` + } + resp := recorder.Body.Bytes() + require.NoError(t, json.Unmarshal(resp, &errResp)) + require.Equal(t, storageErr.Error(), errResp.Error) +} diff --git a/src/query/api/v1/handler/prometheus/native/read_test.go b/src/query/api/v1/handler/prometheus/native/read_test.go index 396a5889c8..e19ebc6bd3 100644 --- a/src/query/api/v1/handler/prometheus/native/read_test.go +++ b/src/query/api/v1/handler/prometheus/native/read_test.go @@ -31,6 +31,7 @@ import ( "time" "github.com/m3db/m3/src/cmd/services/m3query/config" + "github.com/m3db/m3/src/query/api/v1/handler/prometheus" "github.com/m3db/m3/src/query/block" "github.com/m3db/m3/src/query/executor" "github.com/m3db/m3/src/query/models" @@ -49,7 +50,7 @@ func TestPromReadHandler_Read(t *testing.T) { values, bounds := test.GenerateValuesAndBounds(nil, nil) setup := newTestSetup() - promRead := setup.Handler + promRead := setup.Handlers.Read b := test.NewBlockFromValues(bounds, values) setup.Storage.SetFetchBlocksResult(block.Result{Blocks: []block.Block{b}}, nil) @@ -84,7 +85,7 @@ func TestPromReadHandler_ReadM3QL(t *testing.T) { values, bounds := test.GenerateValuesAndBounds(nil, nil) setup := newTestSetup() - promRead := setup.Handler + promRead := setup.Handlers.Read b := test.NewBlockFromValues(bounds, values) setup.Storage.SetFetchBlocksResult(block.Result{Blocks: []block.Block{b}}, nil) @@ -116,29 +117,39 @@ func newReadRequest(t *testing.T, params url.Values) *http.Request { } type testSetup struct { - Storage mock.Storage - Handler *PromReadHandler + Storage mock.Storage + Handlers testSetupHandlers + TimeoutOpts *prometheus.TimeoutOpts +} + +type testSetupHandlers struct { + Read *PromReadHandler + InstantRead *PromReadInstantHandler } func newTestSetup() *testSetup { mockStorage := mock.NewMockStorage() - + scope := tally.NoopScope + engine := executor.NewEngine(mockStorage, scope, + time.Minute, nil) + tagOpts := models.NewTagOptions() + limitsConfig := &config.LimitsConfiguration{} + keepNans := false return &testSetup{ Storage: mockStorage, - Handler: NewPromReadHandler( - executor.NewEngine(mockStorage, tally.NewTestScope("test", nil), time.Minute, nil), - models.NewTagOptions(), - &config.LimitsConfiguration{}, - tally.NewTestScope("", nil), - timeoutOpts, - false, - ), + Handlers: testSetupHandlers{ + Read: NewPromReadHandler(engine, tagOpts, limitsConfig, + scope, timeoutOpts, keepNans), + InstantRead: NewPromReadInstantHandler(engine, tagOpts, + timeoutOpts), + }, + TimeoutOpts: timeoutOpts, } } func TestPromReadHandler_ServeHTTP_maxComputedDatapoints(t *testing.T) { setup := newTestSetup() - setup.Handler.limitsCfg = &config.LimitsConfiguration{ + setup.Handlers.Read.limitsCfg = &config.LimitsConfiguration{ PerQuery: config.PerQueryLimitsConfiguration{ PrivateMaxComputedDatapoints: 3599, }, @@ -151,7 +162,7 @@ func TestPromReadHandler_ServeHTTP_maxComputedDatapoints(t *testing.T) { req := newReadRequest(t, params) recorder := httptest.NewRecorder() - setup.Handler.ServeHTTP(recorder, req) + setup.Handlers.Read.ServeHTTP(recorder, req) resp := recorder.Result() assert.Equal(t, http.StatusBadRequest, resp.StatusCode) @@ -249,13 +260,13 @@ func TestPromReadHandler_validateRequest(t *testing.T) { for _, tc := range cases { t.Run(tc.Name, func(t *testing.T) { setup := newTestSetup() - setup.Handler.limitsCfg = &config.LimitsConfiguration{ + setup.Handlers.Read.limitsCfg = &config.LimitsConfiguration{ PerQuery: config.PerQueryLimitsConfiguration{ PrivateMaxComputedDatapoints: tc.Max, }, } - err := setup.Handler.validateRequest(tc.Params) + err := setup.Handlers.Read.validateRequest(tc.Params) if tc.ErrorExpected { require.Error(t, err) From 551586dde9cbf5804595d2f5d601d4987e27bfde Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Tue, 7 May 2019 16:10:05 -0400 Subject: [PATCH 2/2] Address feedback with construction of handlers --- .../api/v1/handler/prometheus/native/read_test.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/query/api/v1/handler/prometheus/native/read_test.go b/src/query/api/v1/handler/prometheus/native/read_test.go index e19ebc6bd3..110bf8d163 100644 --- a/src/query/api/v1/handler/prometheus/native/read_test.go +++ b/src/query/api/v1/handler/prometheus/native/read_test.go @@ -129,19 +129,25 @@ type testSetupHandlers struct { func newTestSetup() *testSetup { mockStorage := mock.NewMockStorage() + scope := tally.NoopScope engine := executor.NewEngine(mockStorage, scope, time.Minute, nil) tagOpts := models.NewTagOptions() limitsConfig := &config.LimitsConfiguration{} keepNans := false + + read := NewPromReadHandler(engine, tagOpts, limitsConfig, + scope, timeoutOpts, keepNans) + + instantRead := NewPromReadInstantHandler(engine, tagOpts, + timeoutOpts) + return &testSetup{ Storage: mockStorage, Handlers: testSetupHandlers{ - Read: NewPromReadHandler(engine, tagOpts, limitsConfig, - scope, timeoutOpts, keepNans), - InstantRead: NewPromReadInstantHandler(engine, tagOpts, - timeoutOpts), + Read: read, + InstantRead: instantRead, }, TimeoutOpts: timeoutOpts, }