diff --git a/CHANGELOG.md b/CHANGELOG.md index 1686177170..61314ffdb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ This release adds an embedded SQLite database for storing metadata required by t 1. [21936](https://github.com/influxdata/influxdb/pull/21936): Ported the `influxd inspect build-tsi` command from 1.x. 1. [21910](https://github.com/influxdata/influxdb/pull/21910): Added `--ui-disabled` option to `influxd` to allow for running with the UI disabled. 1. [21938](https://github.com/influxdata/influxdb/pull/21938): Added route to delete individual secret. +1. [21958](https://github.com/influxdata/influxdb/pull/21958): Telemetry improvements: Do not record telemetry data for non-existant paths; replace invalid static asset paths with a slug. ### Bug Fixes diff --git a/kit/transport/http/middleware.go b/kit/transport/http/middleware.go index f9b0c0c06c..cd67fea41e 100644 --- a/kit/transport/http/middleware.go +++ b/kit/transport/http/middleware.go @@ -45,14 +45,21 @@ func Metrics(name string, reqMetric *prometheus.CounterVec, durMetric *prometheu statusW := NewStatusResponseWriter(w) defer func(start time.Time) { + statusCode := statusW.Code() + // only log metrics for 2XX or 5XX requests + if !reportFromCode(statusCode) { + return + } + label := prometheus.Labels{ "handler": name, "method": r.Method, "path": normalizePath(r.URL.Path), "status": statusW.StatusCodeClass(), - "response_code": fmt.Sprintf("%d", statusW.Code()), + "response_code": fmt.Sprintf("%d", statusCode), "user_agent": UserAgent(r), } + durMetric.With(label).Observe(time.Since(start).Seconds()) reqMetric.With(label).Inc() }(time.Now()) @@ -239,3 +246,9 @@ func OrgIDFromContext(ctx context.Context) *platform.ID { id := v.(platform.ID) return &id } + +// reportFromCode is a helper function to determine if telemetry data should be +// reported for this response. +func reportFromCode(c int) bool { + return (c >= 200 && c <= 299) || (c >= 500 && c <= 599) +} diff --git a/kit/transport/http/middleware_test.go b/kit/transport/http/middleware_test.go index 058424de15..841097b8b5 100644 --- a/kit/transport/http/middleware_test.go +++ b/kit/transport/http/middleware_test.go @@ -2,15 +2,108 @@ package http import ( "net/http" + "net/http/httptest" "path" "testing" "github.com/influxdata/influxdb/v2/kit/platform" - + "github.com/influxdata/influxdb/v2/kit/prom" + "github.com/influxdata/influxdb/v2/kit/prom/promtest" "github.com/influxdata/influxdb/v2/pkg/testttp" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" ) +func TestMetrics(t *testing.T) { + labels := []string{"handler", "method", "path", "status", "response_code", "user_agent"} + + nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/" { + w.WriteHeader(http.StatusOK) + return + } + + if r.URL.Path == "/serverError" { + w.WriteHeader(http.StatusInternalServerError) + return + } + + if r.URL.Path == "/redirect" { + w.WriteHeader(http.StatusPermanentRedirect) + return + } + + w.WriteHeader(http.StatusNotFound) + }) + + tests := []struct { + name string + reqPath string + wantCount int + labelResponse string + labelStatus string + }{ + { + name: "counter increments on OK (2XX) ", + reqPath: "/", + wantCount: 1, + labelResponse: "200", + labelStatus: "2XX", + }, + { + name: "counter does not increment on not found (4XX)", + reqPath: "/badpath", + wantCount: 0, + }, + { + name: "counter increments on server error (5XX)", + reqPath: "/serverError", + wantCount: 1, + labelResponse: "500", + labelStatus: "5XX", + }, + { + name: "counter does not increment on redirect (3XX)", + reqPath: "/redirect", + wantCount: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + counter := prometheus.NewCounterVec(prometheus.CounterOpts{Name: "counter"}, labels) + hist := prometheus.NewHistogramVec(prometheus.HistogramOpts{Name: "hist"}, labels) + reg := prom.NewRegistry(zaptest.NewLogger(t)) + reg.MustRegister(counter, hist) + + metricsMw := Metrics("testing", counter, hist) + svr := metricsMw(nextHandler) + r := httptest.NewRequest("GET", tt.reqPath, nil) + w := httptest.NewRecorder() + svr.ServeHTTP(w, r) + + mfs := promtest.MustGather(t, reg) + m := promtest.FindMetric(mfs, "counter", map[string]string{ + "handler": "testing", + "method": "GET", + "path": tt.reqPath, + "response_code": tt.labelResponse, + "status": tt.labelStatus, + "user_agent": "unknown", + }) + + if tt.wantCount == 0 { + require.Nil(t, m) + return + } + + require.Equal(t, tt.wantCount, int(m.Counter.GetValue())) + }) + } +} + func Test_normalizePath(t *testing.T) { tests := []struct { name string diff --git a/static/static.go b/static/static.go index b517452187..ec8c67d8ae 100644 --- a/static/static.go +++ b/static/static.go @@ -32,6 +32,11 @@ const ( // swaggerFile is the name of the swagger JSON. swaggerFile = "swagger.json" + + // fallbackPathSlug is the path to re-write on the request if the requested + // path does not match a file and the default file is served. For telemetry + // and metrics reporting purposes. + fallbackPathSlug = "/:fallback_path" ) // NewAssetHandler returns an http.Handler to serve files from the provided @@ -94,25 +99,36 @@ func swaggerHandler(fileOpener http.FileSystem) http.Handler { } // assetHandler returns a handler that either serves the file at that path, or -// the default file if a file cannot be found at that path. +// the default file if a file cannot be found at that path. If the default file +// is served, the request path is re-written to the root path to simplify +// metrics reporting. func assetHandler(fileOpener http.FileSystem) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { name := strings.TrimPrefix(path.Clean(r.URL.Path), "/") // If the root directory is being requested, respond with the default file. if name == "" { name = defaultFile + r.URL.Path = "/" + defaultFile } // Try to open the file requested by name, falling back to the default file. // If even the default file can't be found, the binary must not have been // built with assets, so respond with not found. - f, err := openAsset(fileOpener, name) + f, fallback, err := openAsset(fileOpener, name) if err != nil { http.Error(w, err.Error(), http.StatusNotFound) return } defer f.Close() + // If the default file will be served because the requested path didn't + // match any existing files, re-write the request path a placeholder value. + // This is to ensure that metrics do not get collected for an arbitrarily + // large range of incorrect paths. + if fallback { + r.URL.Path = fallbackPathSlug + } + staticFileHandler(f).ServeHTTP(w, r) } @@ -156,18 +172,21 @@ func staticFileHandler(f fs.File) http.Handler { // openAsset attempts to open the asset by name in the given directory, falling // back to the default file if the named asset can't be found. Returns an error // if even the default asset can't be opened. -func openAsset(fileOpener http.FileSystem, name string) (fs.File, error) { +func openAsset(fileOpener http.FileSystem, name string) (fs.File, bool, error) { + var fallback bool + f, err := fileOpener.Open(name) if err != nil { if os.IsNotExist(err) { + fallback = true f, err = fileOpener.Open(defaultFile) } if err != nil { - return nil, err + return nil, fallback, err } } - return f, nil + return f, fallback, nil } // modTimeFromInfo gets the modification time from an fs.FileInfo. If this diff --git a/static/static_test.go b/static/static_test.go index 47f97a0eda..6f236753de 100644 --- a/static/static_test.go +++ b/static/static_test.go @@ -1,9 +1,11 @@ package static import ( + "io" "io/fs" "io/ioutil" "net/http" + "net/http/httptest" "testing" "testing/fstest" "time" @@ -11,7 +13,75 @@ import ( "github.com/stretchr/testify/require" ) +func TestAssetHandler(t *testing.T) { + t.Parallel() + + defaultData := []byte("this is the default file") + otherData := []byte("this is a different file") + + m := http.FS(fstest.MapFS{ + defaultFile: { + Data: defaultData, + ModTime: time.Now(), + }, + "somethingElse.js": { + Data: otherData, + ModTime: time.Now(), + }, + }) + + tests := []struct { + name string + reqPath string + newPath string + wantData []byte + }{ + { + name: "path matches default", + reqPath: "/" + defaultFile, + newPath: "/" + defaultFile, + wantData: defaultData, + }, + { + name: "root path", + reqPath: "/", + newPath: "/" + defaultFile, + wantData: defaultData, + }, + { + name: "path matches a file", + reqPath: "/somethingElse.js", + newPath: "/somethingElse.js", + wantData: otherData, + }, + { + name: "path matches nothing", + reqPath: "/something_random", + newPath: fallbackPathSlug, + wantData: defaultData, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := assetHandler(m) + r := httptest.NewRequest("GET", tt.reqPath, nil) + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + + b, err := io.ReadAll(w.Result().Body) + require.NoError(t, err) + + require.Equal(t, http.StatusOK, w.Result().StatusCode) + require.Equal(t, tt.wantData, b) + require.Equal(t, tt.newPath, r.URL.Path) + }) + } +} + func TestModTimeFromInfo(t *testing.T) { + t.Parallel() + nowTime := time.Now() timeFunc := func() (time.Time, error) { @@ -76,31 +146,36 @@ func TestOpenAsset(t *testing.T) { }) tests := []struct { - name string - file string - want []byte + name string + file string + fallback bool + want []byte }{ { "default file by name", defaultFile, + false, defaultData, }, { "other file by name", "somethingElse.js", + false, otherData, }, { "falls back to default if can't find", "badFile.exe", + true, defaultData, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotFile, err := openAsset(m, tt.file) + gotFile, fallback, err := openAsset(m, tt.file) require.NoError(t, err) + require.Equal(t, tt.fallback, fallback) got, err := ioutil.ReadAll(gotFile) require.NoError(t, err) @@ -108,7 +183,6 @@ func TestOpenAsset(t *testing.T) { require.Equal(t, tt.want, got) }) } - } func TestEtag(t *testing.T) {