Sync v3 with master (#244)

* Revise internal cardinality metrics (#241)

* Backport #221 and #222 to master branch (#243)

* [bug] Ensure that parent scopes do not return closed subscopes (#221)

* TestScope: don't prune from registry when closed (#222)

---------

Co-authored-by: Matt Way <[email protected]>

---------

Co-authored-by: Matt Way <[email protected]>

generate.go

@@ -1,7 +1,26 @@
+// Copyright (c) 2024 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
 package tally
 
 import (
+	//go:generate mockgen -package tallymock -destination tallymock/stats_reporter.go -imports github.com/uber-go/tally github.com/uber-go/tally StatsReporter
 	_ "github.com/golang/mock/mockgen/model"
 )
-
-//go:generate mockgen -package tallymock -destination tallymock/stats_reporter.go -imports github.com/uber-go/tally github.com/uber-go/tally StatsReporter

m3/example/m3_main.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2021 Uber Technologies, Inc.
+// Copyright (c) 2024 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -87,7 +87,8 @@ func main() {
 	}
 
 	scope, closer := tally.NewRootScope(tally.ScopeOptions{
-		CachedReporter: r,
+		CachedReporter:         r,
+		CardinalityMetricsTags: cfg.M3.InternalTags,
 	}, 1*time.Second)
 
 	defer closer.Close()

m3/reporter.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2021 Uber Technologies, Inc.
+// Copyright (c) 2024 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -66,8 +66,6 @@ const (
 	DefaultHistogramBucketIDName = "bucketid"
 	// DefaultHistogramBucketName is the default histogram bucket name tag name
 	DefaultHistogramBucketName = "bucket"
-	// DefaultTagRedactValue is the default tag value to use when redacting
-	DefaultTagRedactValue = "global"
 	// DefaultHistogramBucketTagPrecision is the default
 	// precision to use when formatting the metric tag
 	// with the histogram bucket bound values.
@@ -290,8 +288,8 @@ func NewReporter(opts Options) (Reporter, error) {
 
 	internalTags := map[string]string{
 		"version":  tally.Version,
-		"host":     DefaultTagRedactValue,
-		"instance": DefaultTagRedactValue,
+		"host":     tally.DefaultTagRedactValue,
+		"instance": tally.DefaultTagRedactValue,
 	}
 
 	for k, v := range opts.InternalTags {

m3/reporter_integration_test.go

@@ -55,6 +55,7 @@ func main() {
 
 	scope, closer := tally.NewRootScope(tally.ScopeOptions{
 		CachedReporter: r,
+		OmitCardinalityMetrics: true,
 	}, 5 * time.Second)
 	defer closer.Close()
 

m3/reporter_test.go

@@ -55,7 +55,7 @@ var defaultCommonTags = map[string]string{"env": "test", "host": "test"}
 var protocols = []Protocol{Compact, Binary}
 
 const internalMetrics = 5    // Additional metrics the reporter sends in a batch - use this, not a magic number.
-const cardinalityMetrics = 3 // Additional metrics emitted by the scope registry.
+const cardinalityMetrics = 4 // Additional metrics emitted by the scope registry.
 
 // TestReporter tests the reporter works as expected with both compact and binary protocols
 func TestReporter(t *testing.T) {
@@ -599,8 +599,8 @@ func TestReporterCommmonTagsInternal(t *testing.T) {
 			}
 
 			// The following tags should be redacted.
-			require.True(t, tagEquals(metric.Tags, "host", DefaultTagRedactValue))
-			require.True(t, tagEquals(metric.Tags, "instance", DefaultTagRedactValue))
+			require.True(t, tagEquals(metric.Tags, "host", tally.DefaultTagRedactValue))
+			require.True(t, tagEquals(metric.Tags, "instance", tally.DefaultTagRedactValue))
 		} else {
 			require.Equal(t, "testCounter1", metric.Name)
 			require.False(t, tagIncluded(metric.Tags, "internal1"))

m3/scope_test.go

@@ -51,10 +51,10 @@ func newTestReporterScope(
 	require.NoError(t, err)
 
 	scope, closer := tally.NewRootScope(tally.ScopeOptions{
-		Prefix:         scopePrefix,
-		Tags:           scopeTags,
-		CachedReporter: r,
-		MetricsOption:  tally.SendInternalMetrics,
+		Prefix:                 scopePrefix,
+		Tags:                   scopeTags,
+		CachedReporter:         r,
+		OmitCardinalityMetrics: false,
 	}, shortInterval)
 
 	return r, scope, func() {

scope.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2023 Uber Technologies, Inc.
+// Copyright (c) 2024 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -28,17 +28,7 @@ import (
 	"go.uber.org/atomic"
 )
 
-// InternalMetricOption is used to configure internal metrics.
-type InternalMetricOption int
-
 const (
-	// Unset is the "no-op" config, which turns off internal metrics.
-	Unset InternalMetricOption = iota
-	// SendInternalMetrics turns on internal metrics submission.
-	SendInternalMetrics
-	// OmitInternalMetrics turns off internal metrics submission.
-	OmitInternalMetrics
-
 	_defaultInitialSliceSize  = 16
 	_defaultReportingInterval = 2 * time.Second
 )
@@ -107,14 +97,15 @@ type scope struct {
 
 // ScopeOptions is a set of options to construct a scope.
 type ScopeOptions struct {
-	Tags            map[string]string
-	Prefix          string
-	Reporter        StatsReporter
-	CachedReporter  CachedStatsReporter
-	Separator       string
-	DefaultBuckets  Buckets
-	SanitizeOptions *SanitizeOptions
-	MetricsOption   InternalMetricOption
+	Tags                   map[string]string
+	Prefix                 string
+	Reporter               StatsReporter
+	CachedReporter         CachedStatsReporter
+	Separator              string
+	DefaultBuckets         Buckets
+	SanitizeOptions        *SanitizeOptions
+	OmitCardinalityMetrics bool
+	CardinalityMetricsTags map[string]string
 
 	testScope          bool
 	registryShardCount uint
@@ -198,7 +189,7 @@ func newRootScope(opts ScopeOptions, interval time.Duration) *scope {
 	s.tags = s.copyAndSanitizeMap(opts.Tags)
 
 	// Register the root scope
-	s.registry = newScopeRegistryWithShardCount(s, opts.registryShardCount, opts.MetricsOption)
+	s.registry = newScopeRegistryWithShardCount(s, opts.registryShardCount, opts.OmitCardinalityMetrics, opts.CardinalityMetricsTags)
 
 	if interval > 0 {
 		s.wg.Add(1)

scope_registry.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2023 Uber Technologies, Inc.
+// Copyright (c) 2024 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -33,10 +33,15 @@ var (
 	scopeRegistryKey = keyForPrefixedStringMaps
 
 	// Metrics related.
-	internalTags             = map[string]string{"version": Version}
-	counterCardinalityName   = "tally_internal_counter_cardinality"
-	gaugeCardinalityName     = "tally_internal_gauge_cardinality"
-	histogramCardinalityName = "tally_internal_histogram_cardinality"
+	counterCardinalityName   = "tally.internal.counter_cardinality"
+	gaugeCardinalityName     = "tally.internal.gauge_cardinality"
+	histogramCardinalityName = "tally.internal.histogram_cardinality"
+	scopeCardinalityName     = "tally.internal.num_active_scopes"
+)
+
+const (
+	// DefaultTagRedactValue is the default tag value to use when redacting
+	DefaultTagRedactValue = "global"
 )
 
 type scopeRegistry struct {
@@ -45,10 +50,16 @@ type scopeRegistry struct {
 	// We need a subscope per GOPROC so that we can take advantage of all the cpu available to the application.
 	subscopes []*scopeBucket
 	// Internal metrics related.
-	internalMetricsOption             InternalMetricOption
+	omitCardinalityMetrics            bool
+	cardinalityMetricsTags            map[string]string
 	sanitizedCounterCardinalityName   string
 	sanitizedGaugeCardinalityName     string
 	sanitizedHistogramCardinalityName string
+	sanitizedScopeCardinalityName     string
+	cachedCounterCardinalityGauge     CachedGauge
+	cachedGaugeCardinalityGauge       CachedGauge
+	cachedHistogramCardinalityGauge   CachedGauge
+	cachedScopeCardinalityGauge       CachedGauge
 }
 
 type scopeBucket struct {
@@ -59,7 +70,8 @@ type scopeBucket struct {
 func newScopeRegistryWithShardCount(
 	root *scope,
 	shardCount uint,
-	internalMetricsOption InternalMetricOption,
+	omitCardinalityMetrics bool,
+	cardinalityMetricsTags map[string]string,
 ) *scopeRegistry {
 	if shardCount == 0 {
 		shardCount = uint(runtime.GOMAXPROCS(-1))
@@ -69,17 +81,34 @@ func newScopeRegistryWithShardCount(
 		root:                              root,
 		subscopes:                         make([]*scopeBucket, shardCount),
 		seed:                              maphash.MakeSeed(),
-		internalMetricsOption:             internalMetricsOption,
+		omitCardinalityMetrics:            omitCardinalityMetrics,
 		sanitizedCounterCardinalityName:   root.sanitizer.Name(counterCardinalityName),
 		sanitizedGaugeCardinalityName:     root.sanitizer.Name(gaugeCardinalityName),
 		sanitizedHistogramCardinalityName: root.sanitizer.Name(histogramCardinalityName),
+		sanitizedScopeCardinalityName:     root.sanitizer.Name(scopeCardinalityName),
+		cardinalityMetricsTags: map[string]string{
+			"version":  Version,
+			"host":     DefaultTagRedactValue,
+			"instance": DefaultTagRedactValue,
+		},
+	}
+
+	for k, v := range cardinalityMetricsTags {
+		r.cardinalityMetricsTags[root.sanitizer.Key(k)] = root.sanitizer.Value(v)
 	}
+
 	for i := uint(0); i < shardCount; i++ {
 		r.subscopes[i] = &scopeBucket{
 			s: make(map[string]*scope),
 		}
 		r.subscopes[i].s[scopeRegistryKey(root.prefix, root.tags)] = root
 	}
+	if r.root.cachedReporter != nil {
+		r.cachedCounterCardinalityGauge = r.root.cachedReporter.AllocateGauge(r.sanitizedCounterCardinalityName, r.cardinalityMetricsTags)
+		r.cachedGaugeCardinalityGauge = r.root.cachedReporter.AllocateGauge(r.sanitizedGaugeCardinalityName, r.cardinalityMetricsTags)
+		r.cachedHistogramCardinalityGauge = r.root.cachedReporter.AllocateGauge(r.sanitizedHistogramCardinalityName, r.cardinalityMetricsTags)
+		r.cachedScopeCardinalityGauge = r.root.cachedReporter.AllocateGauge(r.sanitizedScopeCardinalityName, r.cardinalityMetricsTags)
+	}
 	return r
 }
 
@@ -271,12 +300,13 @@ func (r *scopeRegistry) removeWithRLock(subscopeBucket *scopeBucket, key string)
 
 // Records internal Metrics' cardinalities.
 func (r *scopeRegistry) reportInternalMetrics() {
-	if r.internalMetricsOption != SendInternalMetrics {
+	if r.omitCardinalityMetrics {
 		return
 	}
 
-	counters, gauges, histograms := atomic.Int64{}, atomic.Int64{}, atomic.Int64{}
+	counters, gauges, histograms, scopes := atomic.Int64{}, atomic.Int64{}, atomic.Int64{}, atomic.Int64{}
 	rootCounters, rootGauges, rootHistograms := atomic.Int64{}, atomic.Int64{}, atomic.Int64{}
+	scopes.Inc() // Account for root scope.
 	r.ForEachScope(
 		func(ss *scope) {
 			counterSliceLen, gaugeSliceLen, histogramSliceLen := int64(len(ss.countersSlice)), int64(len(ss.gaugesSlice)), int64(len(ss.histogramsSlice))
@@ -289,25 +319,24 @@ func (r *scopeRegistry) reportInternalMetrics() {
 			counters.Add(counterSliceLen)
 			gauges.Add(gaugeSliceLen)
 			histograms.Add(histogramSliceLen)
+			scopes.Inc()
 		},
 	)
 
 	counters.Add(rootCounters.Load())
 	gauges.Add(rootGauges.Load())
 	histograms.Add(rootHistograms.Load())
-
 	if r.root.reporter != nil {
-		r.root.reporter.ReportCounter(r.sanitizedCounterCardinalityName, internalTags, counters.Load())
-		r.root.reporter.ReportCounter(r.sanitizedGaugeCardinalityName, internalTags, gauges.Load())
-		r.root.reporter.ReportCounter(r.sanitizedHistogramCardinalityName, internalTags, histograms.Load())
+		r.root.reporter.ReportGauge(r.sanitizedCounterCardinalityName, r.cardinalityMetricsTags, float64(counters.Load()))
+		r.root.reporter.ReportGauge(r.sanitizedGaugeCardinalityName, r.cardinalityMetricsTags, float64(gauges.Load()))
+		r.root.reporter.ReportGauge(r.sanitizedHistogramCardinalityName, r.cardinalityMetricsTags, float64(histograms.Load()))
+		r.root.reporter.ReportGauge(r.sanitizedScopeCardinalityName, r.cardinalityMetricsTags, float64(scopes.Load()))
 	}
 
 	if r.root.cachedReporter != nil {
-		numCounters := r.root.cachedReporter.AllocateCounter(r.sanitizedCounterCardinalityName, internalTags)
-		numGauges := r.root.cachedReporter.AllocateCounter(r.sanitizedGaugeCardinalityName, internalTags)
-		numHistograms := r.root.cachedReporter.AllocateCounter(r.sanitizedHistogramCardinalityName, internalTags)
-		numCounters.ReportCount(counters.Load())
-		numGauges.ReportCount(gauges.Load())
-		numHistograms.ReportCount(histograms.Load())
+		r.cachedCounterCardinalityGauge.ReportGauge(float64(counters.Load()))
+		r.cachedGaugeCardinalityGauge.ReportGauge(float64(gauges.Load()))
+		r.cachedHistogramCardinalityGauge.ReportGauge(float64(histograms.Load()))
+		r.cachedScopeCardinalityGauge.ReportGauge(float64(scopes.Load()))
 	}
 }

scope_registry_external_test.go

@@ -113,7 +113,8 @@ func TestNoDefunctSubscopes(t *testing.T) {
 		MinTimes(1)
 
 	root, _ := tally.NewRootScope(tally.ScopeOptions{
-		Reporter: mockreporter,
+		Reporter:               mockreporter,
+		OmitCardinalityMetrics: true,
 	}, time.Millisecond)
 
 	subscope := root.Tagged(tags)