feat(histograms): static config for controlling timer/histogram emission (#7298)

Now that the basics are working: time to allow turning off timers.

Expected use is:
- leave things at default
- add a histogram version of a timer metric (i.e. deploy that code)
- verify new histogram metrics, migrate alerts and dashboards
- the timer data is now unused, so:
- set the timer's key to "histogram" to turn off its emission to save on
computation
  - eventually it will be deleted from the source code (~months later)
- any keys explicitly set to "timer" or "both" will eventually error
when the timer is removed, to prevent missing alerts during the final
migration
- this is not yet implemented, but seems pretty easy. just check
contents against metrics definitions, metrics-defs tests show how.
- "histogram" or empty should not error, we can remove it later (and
force config files to remove the config then too)

And this can also disable histograms if e.g. cardinality is too high and
it causes problems: just set the key to "timer".
Hopefully this is not needed, but if jumping from "no histograms" to
"all timers migrated" in one shot it might be useful for gradual
migration / to reduce large blasts of new metrics.

---

I'd originally considered doing this in dynamic config, but I doubt
we'll need to make quick changes, and static config is significantly
less of a pain, both internally for Reasons™ and in code due to the
key-ID verbosity.

Unrelatedly: this PR is a near-ideal example of why dependency injection
frameworks are nice 😢

---------

Signed-off-by: Steven L <[email protected]>

Author: Groxx

client/wrappers/metered/metered_test.go

@@ -44,7 +44,7 @@ func TestWrappers(t *testing.T) {
 		clientMock := frontend.NewMockClient(ctrl)
 
 		testScope := tally.NewTestScope("", nil)
-		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0))
+		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0), metrics.HistogramMigration{})
 
 		clientMock.EXPECT().CountWorkflowExecutions(gomock.Any(), gomock.Any(), gomock.Any()).
 			Return(nil, nil).Times(1)
@@ -63,7 +63,7 @@ func TestWrappers(t *testing.T) {
 		clientMock := frontend.NewMockClient(ctrl)
 
 		testScope := tally.NewTestScope("", nil)
-		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0))
+		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0), metrics.HistogramMigration{})
 
 		clientMock.EXPECT().CountWorkflowExecutions(gomock.Any(), gomock.Any(), gomock.Any()).
 			Return(nil, errors.New("error"))
@@ -85,7 +85,7 @@ func TestMatching(t *testing.T) {
 		clientMock := matching.NewMockClient(ctrl)
 
 		testScope := tally.NewTestScope("", nil)
-		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0))
+		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0), metrics.HistogramMigration{})
 
 		clientMock.EXPECT().AddActivityTask(gomock.Any(), gomock.Any(), gomock.Any()).
 			Return(&types.AddActivityTaskResponse{}, nil).Times(1)
@@ -106,7 +106,7 @@ func TestMatching(t *testing.T) {
 		clientMock := matching.NewMockClient(ctrl)
 
 		testScope := tally.NewTestScope("", nil)
-		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0))
+		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0), metrics.HistogramMigration{})
 
 		clientMock.EXPECT().PollForDecisionTask(gomock.Any(), gomock.Any(), gomock.Any()).
 			Return(&types.MatchingPollForDecisionTaskResponse{}, nil).Times(1)
@@ -129,7 +129,7 @@ func TestMatching(t *testing.T) {
 		clientMock := matching.NewMockClient(ctrl)
 
 		testScope := tally.NewTestScope("", nil)
-		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0))
+		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0), metrics.HistogramMigration{})
 
 		clientMock.EXPECT().AddActivityTask(gomock.Any(), gomock.Any(), gomock.Any()).
 			Return(&types.AddActivityTaskResponse{}, nil).Times(1)
@@ -150,7 +150,7 @@ func TestMatching(t *testing.T) {
 		clientMock := matching.NewMockClient(ctrl)
 
 		testScope := tally.NewTestScope("", nil)
-		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0))
+		metricsClient := metrics.NewClient(testScope, metrics.ServiceIdx(0), metrics.HistogramMigration{})
 
 		clientMock.EXPECT().AddActivityTask(gomock.Any(), gomock.Any(), gomock.Any()).
 			Return(&types.AddActivityTaskResponse{}, nil).Times(1)

common/archiver/gcloud/historyArchiver_test.go

@@ -63,7 +63,7 @@ func (h *historyArchiverSuite) SetupTest() {
 	h.Assertions = require.New(h.T())
 	h.container = &archiver.HistoryBootstrapContainer{
 		Logger:        testlogger.New(h.T()),
-		MetricsClient: metrics.NewClient(tally.NoopScope, metrics.History),
+		MetricsClient: metrics.NewClient(tally.NoopScope, metrics.History, metrics.HistogramMigration{}),
 	}
 	h.testArchivalURI, _ = archiver.NewURI("gs://my-bucket-cad/cadence_archival/development")
 }

common/archiver/gcloud/visibilityArchiver_test.go

@@ -49,7 +49,7 @@ func (s *visibilityArchiverSuite) SetupTest() {
 	s.Assertions = require.New(s.T())
 	s.container = &archiver.VisibilityBootstrapContainer{
 		Logger:        testlogger.New(s.T()),
-		MetricsClient: metrics.NewClient(tally.NoopScope, metrics.History),
+		MetricsClient: metrics.NewClient(tally.NoopScope, metrics.History, metrics.HistogramMigration{}),
 	}
 	s.expectedVisibilityRecords = []*visibilityRecord{
 		{

common/archiver/s3store/historyArchiver_test.go

@@ -99,7 +99,7 @@ func (s *historyArchiverSuite) SetupTest() {
 	s.Assertions = require.New(s.T())
 	s.container = &archiver.HistoryBootstrapContainer{
 		Logger:        testlogger.New(s.T()),
-		MetricsClient: metrics.NewClient(scope, metrics.History),
+		MetricsClient: metrics.NewClient(scope, metrics.History, metrics.HistogramMigration{}),
 	}
 }
 

common/archiver/s3store/visibilityArchiver_test.go

@@ -124,7 +124,7 @@ func (s *visibilityArchiverSuite) SetupSuite() {
 
 	s.container = &archiver.VisibilityBootstrapContainer{
 		Logger:        testlogger.New(s.T()),
-		MetricsClient: metrics.NewClient(scope, metrics.History),
+		MetricsClient: metrics.NewClient(scope, metrics.History, metrics.HistogramMigration{}),
 	}
 	s.setupVisibilityDirectory()
 }

common/cache/domainCache_test.go

@@ -76,7 +76,7 @@ func (s *domainCacheSuite) SetupTest() {
 	s.logger = testlogger.New(s.Suite.T())
 
 	s.metadataMgr = &mocks.MetadataManager{}
-	metricsClient := metrics.NewClient(tally.NoopScope, metrics.History)
+	metricsClient := metrics.NewClient(tally.NoopScope, metrics.History, metrics.HistogramMigration{})
 	s.domainCache = NewDomainCache(s.metadataMgr, cluster.GetTestClusterMetadata(true), metricsClient, s.logger)
 
 	s.domainCache.timeSource = clock.NewMockedTimeSource()
@@ -1184,7 +1184,7 @@ func Test_WithTimeSource(t *testing.T) {
 	metadataMgr := &mocks.MetadataManager{}
 
 	timeSource := clock.NewRealTimeSource()
-	domainCache := NewDomainCache(metadataMgr, cluster.GetTestClusterMetadata(true), metrics.NewClient(tally.NoopScope, metrics.History), log.NewNoop(), WithTimeSource(timeSource))
+	domainCache := NewDomainCache(metadataMgr, cluster.GetTestClusterMetadata(true), metrics.NewClient(tally.NoopScope, metrics.History, metrics.HistogramMigration{}), log.NewNoop(), WithTimeSource(timeSource))
 
 	assert.Equal(t, timeSource, domainCache.timeSource)
 }

common/cache/metricsScopeCache_test.go

@@ -49,7 +49,7 @@ func TestDomainMetricsCacheSuite(t *testing.T) {
 
 func (s *domainMetricsCacheSuite) SetupTest() {
 	s.Assertions = require.New(s.T())
-	s.metricsClient = metrics.NewClient(tally.NoopScope, metrics.Frontend)
+	s.metricsClient = metrics.NewClient(tally.NoopScope, metrics.Frontend, metrics.HistogramMigration{})
 
 	metricsCache := NewDomainMetricsScopeCache().(*domainMetricsScopeCache)
 	metricsCache.flushDuration = 100 * time.Millisecond

common/config/config.go

@@ -35,6 +35,7 @@ import (
 	"github.com/uber/cadence/common/dynamicconfig"
 	c "github.com/uber/cadence/common/dynamicconfig/configstore/config"
 	"github.com/uber/cadence/common/dynamicconfig/dynamicproperties"
+	"github.com/uber/cadence/common/metrics"
 	"github.com/uber/cadence/common/peerprovider/ringpopprovider"
 	"github.com/uber/cadence/common/service"
 )
@@ -92,6 +93,11 @@ type (
 
 		// ShardDistribution is a config for the shard distributor leader election component that allows to run a single process per region and manage shard namespaces.
 		ShardDistribution ShardDistribution `yaml:"shardDistribution"`
+
+		// Histograms controls timer vs histogram metric emission while they are being migrated.
+		//
+		// Timers will eventually be dropped, and this config will be validation-only (e.g. to error if any explicitly request timers).
+		Histograms metrics.HistogramMigration `yaml:"histograms"`
 	}
 
 	// Membership holds peer provider configuration.

common/config/config_test.go

@@ -22,12 +22,16 @@ package config
 
 import (
 	"net/url"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	uconfig "go.uber.org/config"
+	"gopkg.in/validator.v2"
 
 	"github.com/uber/cadence/common/constants"
+	"github.com/uber/cadence/common/metrics"
 	"github.com/uber/cadence/common/service"
 )
 
@@ -387,3 +391,87 @@ func TestInvalidShardedNoSQLConfig_TasklistShardingRefersToUnknownConnection(t *
 	err := cfg.ValidateAndFillDefaults()
 	require.ErrorContains(t, err, "Unknown tasklist shard name")
 }
+
+func TestHistogramMigrationConfig(t *testing.T) {
+	t.Run("valid", func(t *testing.T) {
+		orig := metrics.HistogramMigrationMetrics
+		t.Cleanup(func() {
+			metrics.HistogramMigrationMetrics = orig
+		})
+		metrics.HistogramMigrationMetrics = map[string]struct{}{
+			"key1": {},
+			"key2": {},
+			"key3": {},
+			"key4": {},
+		}
+
+		// intentionally avoiding the full config struct, as it has other required config
+		yaml, err := uconfig.NewYAML(uconfig.RawSource(strings.NewReader(`
+default: histogram
+names:
+  key1: true
+  key2: false
+  key3:
+`)))
+		require.NoError(t, err)
+
+		var cfg metrics.HistogramMigration
+		err = yaml.Get(uconfig.Root).Populate(&cfg)
+		require.NoError(t, err)
+
+		err = validator.Validate(cfg)
+		require.NoError(t, err)
+
+		check := func(key string, timer, histogram bool) {
+			assert.Equalf(t, timer, cfg.EmitTimer(key), "wrong value for EmitTimer(%q)", key)
+			assert.Equalf(t, histogram, cfg.EmitHistogram(key), "wrong value for EmitHistogram(%q)", key)
+		}
+		check("key1", true, true)
+		check("key2", false, false)
+		check("key3", false, false) // the type's default mode == false.  not truly intended behavior, but it's weird config so it's fine.
+		check("key4", false, true)  // configured default == histogram
+		check("key5", true, true)   // not migrating = always emitted
+		if t.Failed() {
+			t.Logf("config: %#v", cfg)
+		}
+	})
+	t.Run("invalid default", func(t *testing.T) {
+		yaml, err := uconfig.NewYAML(uconfig.RawSource(strings.NewReader(`
+default: xyz
+`)))
+		require.NoError(t, err)
+
+		var cfg metrics.HistogramMigration
+		err = yaml.Get(uconfig.Root).Populate(&cfg)
+		assert.ErrorContains(t, err, `unsupported histogram migration mode "xyz", must be "timer", "histogram", or "both"`)
+	})
+	t.Run("invalid key", func(t *testing.T) {
+		orig := metrics.HistogramMigrationMetrics
+		t.Cleanup(func() {
+			metrics.HistogramMigrationMetrics = orig
+		})
+		metrics.HistogramMigrationMetrics = map[string]struct{}{
+			"key1": {},
+		}
+		yaml, err := uconfig.NewYAML(uconfig.RawSource(strings.NewReader(`
+names:
+  key1: xyz
+`)))
+		require.NoError(t, err)
+
+		var cfg metrics.HistogramMigration
+		err = yaml.Get(uconfig.Root).Populate(&cfg)
+		assert.ErrorContains(t, err, "cannot unmarshal !!str `xyz` into bool")
+	})
+	t.Run("nonexistent key", func(t *testing.T) {
+		yaml, err := uconfig.NewYAML(uconfig.RawSource(strings.NewReader(`
+names:
+  definitely_does_not_exist: true
+`)))
+		require.NoError(t, err)
+
+		var cfg metrics.HistogramMigration
+		err = yaml.Get(uconfig.Root).Populate(&cfg)
+		assert.ErrorContains(t, err, `unknown histogram-migration metric name "definitely_does_not_exist"`)
+	})
+}

common/domain/failover_watcher_test.go

@@ -86,7 +86,7 @@ func (s *failoverWatcherSuite) SetupTest() {
 
 	logger := testlogger.New(s.T())
 	scope := tally.NewTestScope("failover_test", nil)
-	metricsClient := metrics.NewClient(scope, metrics.Frontend)
+	metricsClient := metrics.NewClient(scope, metrics.Frontend, metrics.HistogramMigration{})
 	s.watcher = NewFailoverWatcher(
 		s.mockDomainCache,
 		s.mockMetadataMgr,