feat(telemetry): Do not record telemetry data for invalid requests; r…

…eplace invalid static asset paths with a slug (#21958)

* feat: record telemetry data only on successful response codes

* feat: re-write request path in file handler

* fix: use a slug for the replacement path

* chore: update CHANGELOG

* fix: report for 5XX also

* fix: address review comments

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
 

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)
+}

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

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

static/static_test.go

@@ -1,17 +1,87 @@
 package static
 
 import (
+	"io"
 	"io/fs"
 	"io/ioutil"
 	"net/http"
+	"net/http/httptest"
 	"testing"
 	"testing/fstest"
 	"time"
 
 	"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,39 +146,43 @@ 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)
 
 			require.Equal(t, tt.want, got)
 		})
 	}
-
 }
 
 func TestEtag(t *testing.T) {