From 61603cfaa8ade01bda669a5ea336d3ab643f699f Mon Sep 17 00:00:00 2001 From: Dan Moran Date: Mon, 15 Mar 2021 14:01:33 -0400 Subject: [PATCH 1/7] refactor: move common HTTP error-writing code into new function --- kit/transport/http/error_handler.go | 22 +++++++++++++------- pprof/http_server.go | 31 +++++++---------------------- 2 files changed, 22 insertions(+), 31 deletions(-) diff --git a/kit/transport/http/error_handler.go b/kit/transport/http/error_handler.go index da57d8dc4a0..cdb13693947 100644 --- a/kit/transport/http/error_handler.go +++ b/kit/transport/http/error_handler.go @@ -27,18 +27,26 @@ func (h ErrorHandler) HandleHTTPError(ctx context.Context, err error, w http.Res } code := influxdb.ErrorCode(err) + var msg string + if err, ok := err.(*influxdb.Error); ok { + msg = err.Error() + } else { + msg = "An internal error has occurred" + } + + WriteErrorResponse(ctx, w, code, msg) +} + +func WriteErrorResponse(ctx context.Context, w http.ResponseWriter, code string, msg string) { w.Header().Set(PlatformErrorCodeHeader, code) w.Header().Set("Content-Type", "application/json; charset=utf-8") w.WriteHeader(ErrorCodeToStatusCode(ctx, code)) - var e struct { + e := struct { Code string `json:"code"` Message string `json:"message"` - } - e.Code = influxdb.ErrorCode(err) - if err, ok := err.(*influxdb.Error); ok { - e.Message = err.Error() - } else { - e.Message = "An internal error has occurred" + }{ + Code: code, + Message: msg, } b, _ := json.Marshal(e) _, _ = w.Write(b) diff --git a/pprof/http_server.go b/pprof/http_server.go index 7702178bb18..b7a4175ed97 100644 --- a/pprof/http_server.go +++ b/pprof/http_server.go @@ -1,8 +1,6 @@ package pprof import ( - "context" - "encoding/json" "fmt" "io" "net/http" @@ -37,23 +35,8 @@ func NewHTTPHandler(profilingEnabled bool) *Handler { return &Handler{r} } -func errResponse(ctx context.Context, w http.ResponseWriter, code string, message string) { - w.Header().Set(ihttp.PlatformErrorCodeHeader, code) - w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.WriteHeader(ihttp.ErrorCodeToStatusCode(ctx, code)) - e := struct { - Code string `json:"code"` - Message string `json:"message"` - }{ - Code: code, - Message: message, - } - b, _ := json.Marshal(e) - _, _ = w.Write(b) -} - func profilingDisabledHandler(w http.ResponseWriter, r *http.Request) { - errResponse(r.Context(), w, influxdb.EForbidden, "profiling disabled") + ihttp.WriteErrorResponse(r.Context(), w, influxdb.EForbidden, "profiling disabled") } func archiveProfilesHandler(w http.ResponseWriter, r *http.Request) { @@ -65,7 +48,7 @@ func archiveProfilesHandler(w http.ResponseWriter, r *http.Request) { // distinguish between a form value that exists and has no value and one that // does not exist at all. if err := r.ParseForm(); err != nil { - errResponse(ctx, w, influxdb.EInternal, err.Error()) + ihttp.WriteErrorResponse(ctx, w, influxdb.EInternal, err.Error()) return } @@ -112,18 +95,18 @@ func archiveProfilesHandler(w http.ResponseWriter, r *http.Request) { // In this case it is a StatusBadRequest (400) since the problem is in the // supplied form data. if duration < 0 { - errResponse(ctx, w, influxdb.EInvalid, "negative trace durations not allowed") + ihttp.WriteErrorResponse(ctx, w, influxdb.EInvalid, "negative trace durations not allowed") return } if err != nil { - errResponse(ctx, w, influxdb.EInvalid, fmt.Sprintf("could not parse supplied duration for trace %q", val)) + ihttp.WriteErrorResponse(ctx, w, influxdb.EInvalid, fmt.Sprintf("could not parse supplied duration for trace %q", val)) return } // Trace files can get big. Lets clamp the maximum trace duration to 45s. if duration > 45*time.Second { - errResponse(ctx, w, influxdb.EInvalid, "cannot trace for longer than 45s") + ihttp.WriteErrorResponse(ctx, w, influxdb.EInvalid, "cannot trace for longer than 45s") return } @@ -172,7 +155,7 @@ func archiveProfilesHandler(w http.ResponseWriter, r *http.Request) { duration, err := getDuration() if err != nil { - errResponse(ctx, w, influxdb.EInvalid, fmt.Sprintf("could not parse supplied duration for cpu profile %q", val)) + ihttp.WriteErrorResponse(ctx, w, influxdb.EInvalid, fmt.Sprintf("could not parse supplied duration for cpu profile %q", val)) return } @@ -181,7 +164,7 @@ func archiveProfilesHandler(w http.ResponseWriter, r *http.Request) { tarstream, err := collectAllProfiles(ctx, traceDuration, cpuDuration) if err != nil { - errResponse(ctx, w, influxdb.EInternal, err.Error()) + ihttp.WriteErrorResponse(ctx, w, influxdb.EInternal, err.Error()) return } _, _ = io.Copy(w, tarstream) From ba4364fb6d10c7fad05e975868528f1fcd96e55a Mon Sep 17 00:00:00 2001 From: Dan Moran Date: Mon, 15 Mar 2021 14:01:54 -0400 Subject: [PATCH 2/7] feat(http): add config option to disable metrics endpoint in influxd --- cmd/influxd/launcher/cmd.go | 10 +++++ cmd/influxd/launcher/launcher.go | 4 +- http/handler.go | 68 +++++++++++++++++--------------- http/handler_test.go | 4 +- 4 files changed, 51 insertions(+), 35 deletions(-) diff --git a/cmd/influxd/launcher/cmd.go b/cmd/influxd/launcher/cmd.go index 584b44b458e..95ee418f327 100644 --- a/cmd/influxd/launcher/cmd.go +++ b/cmd/influxd/launcher/cmd.go @@ -143,6 +143,7 @@ type InfluxdOpts struct { SessionRenewDisabled bool ProfilingDisabled bool + MetricsDisabled bool NatsPort int NatsMaxPayloadBytes int @@ -189,6 +190,7 @@ func newOpts(viper *viper.Viper) *InfluxdOpts { SessionRenewDisabled: false, ProfilingDisabled: false, + MetricsDisabled: false, StoreType: BoltStore, SecretStore: BoltStore, @@ -519,5 +521,13 @@ func (o *InfluxdOpts) bindCliOpts() []cli.Opt { Desc: "Don't expose debugging information over HTTP at /debug/pprof", Default: o.ProfilingDisabled, }, + + // Metrics config + { + DestP: &o.MetricsDisabled, + Flag: "metrics-disabled", + Desc: "Don't expose metrics over HTTP at /metrics", + Default: o.MetricsDisabled, + }, } } diff --git a/cmd/influxd/launcher/launcher.go b/cmd/influxd/launcher/launcher.go index 0fac36c1f52..3d3c7764191 100644 --- a/cmd/influxd/launcher/launcher.go +++ b/cmd/influxd/launcher/launcher.go @@ -915,12 +915,12 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) { ) httpLogger := m.log.With(zap.String("service", "http")) - m.httpServer.Handler = http.NewHandlerFromRegistry( + m.httpServer.Handler = http.NewRootHandler( "platform", - m.reg, http.WithLog(httpLogger), http.WithAPIHandler(platformHandler), http.WithPprofEnabled(!opts.ProfilingDisabled), + http.WithMetrics(m.reg, !opts.MetricsDisabled), ) if opts.LogLevel == zap.DebugLevel { diff --git a/http/handler.go b/http/handler.go index de700133a8a..63634760267 100644 --- a/http/handler.go +++ b/http/handler.go @@ -6,6 +6,7 @@ import ( "net/http" "github.com/go-chi/chi" + "github.com/influxdata/influxdb/v2" "github.com/influxdata/influxdb/v2/kit/prom" kithttp "github.com/influxdata/influxdb/v2/kit/transport/http" "github.com/influxdata/influxdb/v2/pprof" @@ -39,17 +40,31 @@ type Handler struct { type ( handlerOpts struct { - log *zap.Logger - apiHandler http.Handler - healthHandler http.Handler - metricsHandler http.Handler - readyHandler http.Handler - pprofEnabled bool + log *zap.Logger + apiHandler http.Handler + healthHandler http.Handler + readyHandler http.Handler + pprofEnabled bool + + // NOTE: Track the registry even if metricsExposed = false + // so we can report HTTP metrics via telemetry. + metricsRegistry *prom.Registry + metricsExposed bool } HandlerOptFn func(opts *handlerOpts) ) +func (o *handlerOpts) metricsHTTPHandler() http.Handler { + if !o.metricsExposed || o.metricsRegistry == nil { + handlerFunc := func(rw http.ResponseWriter, r *http.Request) { + kithttp.WriteErrorResponse(r.Context(), rw, influxdb.EForbidden, "metrics disabled") + } + return http.HandlerFunc(handlerFunc) + } + return o.metricsRegistry.HTTPHandler() +} + func WithLog(l *zap.Logger) HandlerOptFn { return func(opts *handlerOpts) { opts.log = l @@ -68,34 +83,23 @@ func WithPprofEnabled(enabled bool) HandlerOptFn { } } -func WithHealthHandler(h http.Handler) HandlerOptFn { - return func(opts *handlerOpts) { - opts.healthHandler = h - } -} - -func WithMetricsHandler(h http.Handler) HandlerOptFn { - return func(opts *handlerOpts) { - opts.metricsHandler = h - } -} - -func WithReadyHandler(h http.Handler) HandlerOptFn { +func WithMetrics(reg *prom.Registry, exposed bool) HandlerOptFn { return func(opts *handlerOpts) { - opts.readyHandler = h + opts.metricsRegistry = reg + opts.metricsExposed = exposed } } -// NewHandlerFromRegistry creates a new handler with the given name, -// and sets the /metrics endpoint to use the metrics from the given registry, -// after self-registering h's metrics. -func NewHandlerFromRegistry(name string, reg *prom.Registry, opts ...HandlerOptFn) *Handler { +// NewRootHandler creates a new handler with the given name and registers any root-level +// (non-API) routes enabled by the caller. +func NewRootHandler(name string, opts ...HandlerOptFn) *Handler { opt := handlerOpts{ - log: zap.NewNop(), - healthHandler: http.HandlerFunc(HealthHandler), - metricsHandler: reg.HTTPHandler(), - readyHandler: ReadyHandler(), - pprofEnabled: false, + log: zap.NewNop(), + healthHandler: http.HandlerFunc(HealthHandler), + readyHandler: ReadyHandler(), + pprofEnabled: false, + metricsRegistry: nil, + metricsExposed: false, } for _, o := range opts { o(&opt) @@ -113,7 +117,7 @@ func NewHandlerFromRegistry(name string, reg *prom.Registry, opts ...HandlerOptF r.Use( kithttp.Metrics(name, h.requests, h.requestDur), ) - r.Mount(MetricsPath, opt.metricsHandler) + r.Mount(MetricsPath, opt.metricsHTTPHandler()) r.Mount(ReadyPath, opt.readyHandler) r.Mount(HealthPath, opt.healthHandler) r.Mount(DebugPath, pprof.NewHTTPHandler(opt.pprofEnabled)) @@ -130,7 +134,9 @@ func NewHandlerFromRegistry(name string, reg *prom.Registry, opts ...HandlerOptF h.r = r - reg.MustRegister(h.PrometheusCollectors()...) + if opt.metricsRegistry != nil { + opt.metricsRegistry.MustRegister(h.PrometheusCollectors()...) + } return h } diff --git a/http/handler_test.go b/http/handler_test.go index bbf0c5015ac..9f74750e512 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -42,11 +42,11 @@ func TestHandler_ServeHTTP(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { reg := prom.NewRegistry(zaptest.NewLogger(t)) - h := NewHandlerFromRegistry( + h := NewRootHandler( tt.fields.name, - reg, WithLog(tt.fields.log), WithAPIHandler(tt.fields.handler), + WithMetrics(reg, true), ) tt.args.r.Header.Set("User-Agent", "ua1") From 339020fddad5ce427c2a780c4b0a279a1ee81f66 Mon Sep 17 00:00:00 2001 From: Dan Moran Date: Mon, 15 Mar 2021 14:11:06 -0400 Subject: [PATCH 3/7] chore: update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe16c520cd8..af5c97b00f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ or `/query` HTTP endpoints. 1. [20827](https://github.com/influxdata/influxdb/pull/20827): Upgrade `http.pprof-enabled` config in `influxd upgrade`. 1. [20911](https://github.com/influxdata/influxdb/pull/20911): Add support for explicitly setting shard-group durations on buckets. Thanks @hinst! 1. [20882](https://github.com/influxdata/influxdb/pull/20882): Rewrite regex conditions in InfluxQL subqueries for performance. Thanks @yujiahaol68! +1. [20963](https://github.com/influxdata/influxdb/pull/20963): Add `--metrics-disabled` option to `influxd` to disable exposing Prometheus metrics over HTTP. ### Bug Fixes From 561052acdf6d1f1da7891e23dee66b7091b38b0c Mon Sep 17 00:00:00 2001 From: Dan Moran Date: Mon, 15 Mar 2021 14:43:09 -0400 Subject: [PATCH 4/7] test: add test case for hidden metrics --- http/handler_test.go | 53 +++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/http/handler_test.go b/http/handler_test.go index 9f74750e512..6c5ee16eae2 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -7,24 +7,22 @@ import ( "github.com/influxdata/influxdb/v2/kit/prom" "github.com/influxdata/influxdb/v2/kit/prom/promtest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/zap" "go.uber.org/zap/zaptest" ) func TestHandler_ServeHTTP(t *testing.T) { type fields struct { - name string - handler http.Handler - log *zap.Logger - } - type args struct { - w *httptest.ResponseRecorder - r *http.Request + name string + handler http.Handler + handlerHidden bool + log *zap.Logger } tests := []struct { name string fields fields - args args }{ { name: "should record metrics when http handling", @@ -33,9 +31,14 @@ func TestHandler_ServeHTTP(t *testing.T) { handler: http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), log: zaptest.NewLogger(t), }, - args: args{ - r: httptest.NewRequest(http.MethodGet, "/", nil), - w: httptest.NewRecorder(), + }, + { + name: "should record metrics when http handling", + fields: fields{ + name: "test", + handler: http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), + handlerHidden: true, + log: zaptest.NewLogger(t), }, }, } @@ -46,16 +49,15 @@ func TestHandler_ServeHTTP(t *testing.T) { tt.fields.name, WithLog(tt.fields.log), WithAPIHandler(tt.fields.handler), - WithMetrics(reg, true), + WithMetrics(reg, !tt.fields.handlerHidden), ) - tt.args.r.Header.Set("User-Agent", "ua1") - h.ServeHTTP(tt.args.w, tt.args.r) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("User-Agent", "ua1") + h.ServeHTTP(httptest.NewRecorder(), req) mfs, err := reg.Gather() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) c := promtest.MustFindMetric(t, mfs, "http_api_requests_total", map[string]string{ "handler": "test", @@ -65,9 +67,7 @@ func TestHandler_ServeHTTP(t *testing.T) { "user_agent": "ua1", "response_code": "200", }) - if got := c.GetCounter().GetValue(); got != 1 { - t.Fatalf("expected counter to be 1, got %v", got) - } + require.Equal(t, 1, int(c.GetCounter().GetValue())) g := promtest.MustFindMetric(t, mfs, "http_api_request_duration_seconds", map[string]string{ "handler": "test", @@ -77,10 +77,17 @@ func TestHandler_ServeHTTP(t *testing.T) { "user_agent": "ua1", "response_code": "200", }) - if got := g.GetHistogram().GetSampleCount(); got != 1 { - t.Fatalf("expected histogram sample count to be 1, got %v", got) + require.Equal(t, 1, int(g.GetHistogram().GetSampleCount())) + + req = httptest.NewRequest(http.MethodGet, "/metrics", nil) + recorder := httptest.NewRecorder() + h.ServeHTTP(recorder, req) + + if tt.fields.handlerHidden { + assert.Equal(t, http.StatusForbidden, recorder.Code) + } else { + assert.Equal(t, http.StatusOK, recorder.Code) } }) - } } From 7dc1310785e00ccc8e3fee48690b4fe9e8681257 Mon Sep 17 00:00:00 2001 From: Dan Moran Date: Mon, 15 Mar 2021 14:45:03 -0400 Subject: [PATCH 5/7] chore: use `require`, not `assert` --- http/handler_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/http/handler_test.go b/http/handler_test.go index 6c5ee16eae2..6ef55755ab1 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -7,7 +7,6 @@ import ( "github.com/influxdata/influxdb/v2/kit/prom" "github.com/influxdata/influxdb/v2/kit/prom/promtest" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" "go.uber.org/zap/zaptest" @@ -84,9 +83,9 @@ func TestHandler_ServeHTTP(t *testing.T) { h.ServeHTTP(recorder, req) if tt.fields.handlerHidden { - assert.Equal(t, http.StatusForbidden, recorder.Code) + require.Equal(t, http.StatusForbidden, recorder.Code) } else { - assert.Equal(t, http.StatusOK, recorder.Code) + require.Equal(t, http.StatusOK, recorder.Code) } }) } From b68c46281ba4831da83c52caf8526dd3ade02ee6 Mon Sep 17 00:00:00 2001 From: Dan Moran Date: Mon, 15 Mar 2021 14:48:02 -0400 Subject: [PATCH 6/7] refactor: clean up conditional logic --- http/handler.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/http/handler.go b/http/handler.go index 63634760267..6fd280e9849 100644 --- a/http/handler.go +++ b/http/handler.go @@ -56,13 +56,13 @@ type ( ) func (o *handlerOpts) metricsHTTPHandler() http.Handler { - if !o.metricsExposed || o.metricsRegistry == nil { - handlerFunc := func(rw http.ResponseWriter, r *http.Request) { - kithttp.WriteErrorResponse(r.Context(), rw, influxdb.EForbidden, "metrics disabled") - } - return http.HandlerFunc(handlerFunc) + if o.metricsRegistry != nil && o.metricsExposed { + return o.metricsRegistry.HTTPHandler() } - return o.metricsRegistry.HTTPHandler() + handlerFunc := func(rw http.ResponseWriter, r *http.Request) { + kithttp.WriteErrorResponse(r.Context(), rw, influxdb.EForbidden, "metrics disabled") + } + return http.HandlerFunc(handlerFunc) } func WithLog(l *zap.Logger) HandlerOptFn { From ae96e905d1dfaf21723a3ad953c3d6e9a6a40e2e Mon Sep 17 00:00:00 2001 From: Dan Moran Date: Mon, 15 Mar 2021 14:48:49 -0400 Subject: [PATCH 7/7] test: fix duplicate test name --- http/handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http/handler_test.go b/http/handler_test.go index 6ef55755ab1..783e937b9db 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -32,7 +32,7 @@ func TestHandler_ServeHTTP(t *testing.T) { }, }, { - name: "should record metrics when http handling", + name: "should record metrics even when not exposed over HTTP", fields: fields{ name: "test", handler: http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}),