Disable in-mem telemetry collection by default

conf/server/server_full.conf

@@ -801,7 +801,7 @@ plugins {
 #     ]
 
 #     InMem {
-#         # enabled: Enable this collector. Default: true.
+#         # enabled: Enable this collector. This field is deprecated and will be removed in a future release. Default: false.
 #         # enabled = true
 #     }
 # }

doc/telemetry_config.md

@@ -51,9 +51,9 @@ You may use all, some, or none of the collectors. The following collectors suppo
 | `env`         | `string` | M3 environment, e.g. `production`, `staging` |
 
 #### `In-Mem`
-| Configuration | Type   | Description           | Default |
-|---------------|--------|-----------------------|---------|
-| `enabled`     | `bool` | Enable this collector | `true`  |
+| Configuration | Type   | Description                                                                                                                                                                    | Default |
+|---------------|--------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------|
+| `enabled`     | `bool` | Enable this collector. This flag is deprecated and will be removed in a future release. To disable in-memory telemetry collection omit the InMem configuration block entirely. | `false` |
 
 Here is a sample configuration:
 
@@ -76,9 +76,7 @@ telemetry {
             { address = "localhost:9000" env = "prod" },
         ]
 
-        InMem {
-            enabled = false
-        }
+        InMem {}
 
         AllowedLabels = []
         BlockedLabels = []

pkg/common/telemetry/config.go

@@ -49,6 +49,7 @@ type M3Config struct {
 }
 
 type InMem struct {
-	Enabled    *bool    `hcl:"enabled"`
-	UnusedKeys []string `hcl:",unusedKeys"`
+	// TODO: remove in SPIRE 1.6.0
+	DeprecatedEnabled *bool    `hcl:"enabled"`
+	UnusedKeys        []string `hcl:",unusedKeys"`
 }

pkg/common/telemetry/inmem.go

@@ -3,9 +3,6 @@ package telemetry
 import (
 	"context"
 	"io"
-	"os"
-	"os/signal"
-	"sync"
 	"time"
 
 	"github.com/armon/go-metrics"
@@ -21,19 +18,23 @@ type inmemRunner struct {
 	log        logrus.FieldLogger
 	w          io.Writer
 	loadedSink *metrics.InmemSink
-
-	inMemBlockSet bool
 }
 
 func newInmemRunner(c *MetricsConfig) (sinkRunner, error) {
 	runner := &inmemRunner{
 		log: c.Logger,
 	}
 
-	if c.FileConfig.InMem != nil && c.FileConfig.InMem.Enabled != nil {
-		runner.inMemBlockSet = true
-
-		if !*c.FileConfig.InMem.Enabled {
+	// Don't enable If the InMem block is not present, or is present with
+	// the deprecated "enabled" flag explicitly set to false.
+	// TODO: Remove the deprecated "enabled" flag in 1.6.0.
+	inMem := c.FileConfig.InMem
+	switch {
+	case inMem == nil:
+		return runner, nil
+	case inMem.DeprecatedEnabled != nil:
+		c.Logger.Warn("The enabled flag is deprecated in the InMem configuration and will be removed in a future release; omit the InMem block to disable in-memory telemetry")
+		if !*inMem.DeprecatedEnabled {
 			return runner, nil
 		}
 	}
@@ -66,45 +67,10 @@ func (i *inmemRunner) run(ctx context.Context) error {
 		return nil
 	}
 
-	var wg sync.WaitGroup
-
-	i.startInMemMetrics(ctx, &wg)
-
-	if !i.inMemBlockSet {
-		i.startConfigWarning(ctx, &wg)
-	}
-
-	wg.Wait()
-	return nil
-}
-
-func (i *inmemRunner) startConfigWarning(ctx context.Context, wg *sync.WaitGroup) {
-	wg.Add(1)
-	sigChannel := make(chan os.Signal, 1)
-	signal.Notify(sigChannel, metrics.DefaultSignal)
-	go func() {
-		defer wg.Done()
-		for {
-			select {
-			case <-sigChannel:
-				i.log.Warn("The in-memory telemetry sink will be disabled by default in a future release." +
-					" If you wish to continue using it, please enable it in the telemetry configuration")
-			case <-ctx.Done():
-				signal.Stop(sigChannel)
-				return
-			}
-		}
-	}()
-}
-
-func (i *inmemRunner) startInMemMetrics(ctx context.Context, wg *sync.WaitGroup) {
-	wg.Add(1)
 	signalHandler := metrics.NewInmemSignal(i.loadedSink, metrics.DefaultSignal, i.w)
-	go func() {
-		defer wg.Done()
-		<-ctx.Done()
-		signalHandler.Stop()
-	}()
+	defer signalHandler.Stop()
+	<-ctx.Done()
+	return nil
 }
 
 func (i *inmemRunner) requiresTypePrefix() bool {

pkg/common/telemetry/inmem_test.go

@@ -1,162 +1,116 @@
 package telemetry
 
 import (
-	"context"
-	"os"
-	"runtime"
 	"testing"
-	"time"
 
-	"github.com/armon/go-metrics"
 	"github.com/sirupsen/logrus"
 	"github.com/sirupsen/logrus/hooks/test"
-	"github.com/spiffe/spire/test/util"
+	"github.com/spiffe/spire/test/spiretest"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func TestNewInmemRunner(t *testing.T) {
-	config := testInmemConfig()
-	_, err := newInmemRunner(config)
-	assert.Nil(t, err)
-}
-
-func TestDefaultEnabledNewInmemRunner(t *testing.T) {
-	t.Run("enabled when block undeclared", func(t *testing.T) {
-		runner, err := newInmemRunner(testInmemConfig())
-		assert.Nil(t, err)
-		assert.True(t, runner.isConfigured())
-	})
-
-	t.Run("enabled flag undeclared", func(t *testing.T) {
-		config := testInmemConfig()
-		config.FileConfig = FileConfig{
-			InMem: &InMem{},
-		}
-		runner, err := newInmemRunner(config)
-		assert.Nil(t, err)
-		assert.True(t, runner.isConfigured())
-	})
-
-	t.Run("enabled flag declared", func(t *testing.T) {
-		enabledFlag := true
-
-		config := testInmemConfig()
-		config.FileConfig = FileConfig{
-			InMem: &InMem{
-				Enabled: &enabledFlag,
+func TestInMem(t *testing.T) {
+	enabled := true
+	disabled := false
+
+	for _, tt := range []struct {
+		test               string
+		inMemConfig        *InMem
+		removeLoggerWriter bool
+		expectErr          string
+		expectEnabled      bool
+		expectLogs         []spiretest.LogEntry
+	}{
+		{
+			test:          "disabled when InMem block undeclared",
+			inMemConfig:   nil,
+			expectEnabled: false,
+		},
+		{
+			test:          "enabled when InMem block declared but deprecated enabled flag unset",
+			inMemConfig:   &InMem{},
+			expectEnabled: true,
+		},
+		{
+			test:          "enabled when InMem block declared and deprecated enabled flag set to true",
+			inMemConfig:   &InMem{DeprecatedEnabled: &enabled},
+			expectEnabled: true,
+			expectLogs: []spiretest.LogEntry{
+				{
+					Level:   logrus.WarnLevel,
+					Message: "The enabled flag is deprecated in the InMem configuration and will be removed in a future release; omit the InMem block to disable in-memory telemetry",
+				},
 			},
-		}
-		runner, err := newInmemRunner(config)
-		assert.Nil(t, err)
-		assert.True(t, runner.isConfigured())
-	})
-}
-
-func TestDisabledNewInmemRunner(t *testing.T) {
-	enabledFlag := false
-
-	config := &MetricsConfig{
-		ServiceName: "foo",
-		FileConfig: FileConfig{
-			InMem: &InMem{
-				Enabled: &enabledFlag,
+		},
+		{
+			test:          "disabled when InMem block declared and deprecated enabled flag set to false",
+			inMemConfig:   &InMem{DeprecatedEnabled: &disabled},
+			expectEnabled: false,
+			expectLogs: []spiretest.LogEntry{
+				{
+					Level:   logrus.WarnLevel,
+					Message: "The enabled flag is deprecated in the InMem configuration and will be removed in a future release; omit the InMem block to disable in-memory telemetry",
+				},
 			},
 		},
-	}
-	runner, err := newInmemRunner(config)
-	assert.Nil(t, err)
-	assert.False(t, runner.isConfigured())
-}
-
-func TestWarnOnFutureDisable(t *testing.T) {
-	// It is not possible to send signals to process on windows except for kill
-	if runtime.GOOS == "windows" {
-		t.Skip()
-	}
-	logger, hook := test.NewNullLogger()
-
-	// Get a real logrus.Entry
-	logger.SetLevel(logrus.DebugLevel)
-	c := &MetricsConfig{
-		Logger:      logger,
-		ServiceName: "foo",
-	}
-
-	ir, err := newInmemRunner(c)
-	require.Nil(t, err)
-	require.Equal(t, 1, len(ir.sinks()))
-
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	errCh := make(chan error, 1)
-	go func() {
-		errCh <- ir.run(ctx)
-	}()
-
-	// Send signal, wait for signal handling + logging
-	util.RunWithTimeout(t, time.Minute, func() {
-		for {
-			p, err := os.FindProcess(os.Getpid())
-			require.NoError(t, err)
-			require.NoError(t, p.Signal(metrics.DefaultSignal))
-
-			require.NoError(t, ctx.Err())
+		{
+			test:               "disabled when unexpected logger passed",
+			inMemConfig:        &InMem{},
+			removeLoggerWriter: true,
+			expectEnabled:      false,
+			expectLogs: []spiretest.LogEntry{
+				{
+					Level:   logrus.WarnLevel,
+					Message: "Unknown logging subsystem; disabling telemetry signaling",
+				},
+			},
+		},
+	} {
+		t.Run(tt.test, func(t *testing.T) {
+			var logger logrus.FieldLogger
+			var hook *test.Hook
+			logger, hook = test.NewNullLogger()
+			if tt.removeLoggerWriter {
+				logger = noWriterLogger(logger)
+			}
 
-			if entry := hook.LastEntry(); entry != nil {
-				assert.Equal(t, "The in-memory telemetry sink will be disabled by default in a future release."+
-					" If you wish to continue using it, please enable it in the telemetry configuration", entry.Message)
+			runner, err := newInmemRunner(&MetricsConfig{
+				Logger:      logger,
+				ServiceName: "foo",
+				FileConfig:  FileConfig{InMem: tt.inMemConfig},
+			})
+			if tt.expectErr != "" {
+				require.EqualError(t, err, tt.expectErr)
+				assert.Nil(t, runner)
 				return
 			}
-		}
-	})
-
-	cancel()
-	require.NoError(t, <-errCh)
-}
-
-func TestInmemSinks(t *testing.T) {
-	ir, err := newInmemRunner(testUnknownInmemConfig())
-	require.Nil(t, err)
-	assert.Equal(t, 0, len(ir.sinks()))
-
-	ir, err = newInmemRunner(testInmemConfig())
-	require.Nil(t, err)
-	assert.Equal(t, 1, len(ir.sinks()))
-}
 
-func TestInmemIsConfigured(t *testing.T) {
-	ir, err := newInmemRunner(testInmemConfig())
-	require.Nil(t, err)
-	assert.True(t, ir.isConfigured())
+			require.NoError(t, err)
+			if tt.expectEnabled {
+				assert.True(t, runner.isConfigured())
+				assert.Len(t, runner.sinks(), 1)
+			} else {
+				assert.False(t, runner.isConfigured())
+				assert.Len(t, runner.sinks(), 0)
+			}
 
-	ir, err = newInmemRunner(testUnknownInmemConfig())
-	require.Nil(t, err)
-	assert.False(t, ir.isConfigured())
+			spiretest.AssertLogs(t, hook.AllEntries(), tt.expectLogs)
+		})
+	}
 }
 
 func testInmemConfig() *MetricsConfig {
-	l, _ := test.NewNullLogger()
+	logger, _ := test.NewNullLogger()
 	return &MetricsConfig{
-		Logger:      l,
+		Logger:      logger,
 		ServiceName: "foo",
+		FileConfig:  FileConfig{InMem: &InMem{}},
 	}
 }
 
-func testUnknownInmemConfig() *MetricsConfig {
-	l, _ := test.NewNullLogger()
-
-	// unknownLogger only provides logrus.FieldLogger interface and does not give
-	// access to the underlying writer via the Writer() method.
-	unknownLogger := struct {
-		logrus.FieldLogger
-	}{
-		FieldLogger: l,
-	}
-
-	return &MetricsConfig{
-		Logger:      unknownLogger,
-		ServiceName: "foo",
-	}
+func noWriterLogger(logger logrus.FieldLogger) logrus.FieldLogger {
+	// Hide the type of the underlying logger to hide the io.Writer
+	// implementation
+	return struct{ logrus.FieldLogger }{FieldLogger: logger}
 }