From 24b844bfdc627df021d0fee64b2d17a982224cc7 Mon Sep 17 00:00:00 2001 From: brawndou <112038567+brawndou@users.noreply.github.com> Date: Thu, 26 Jan 2023 15:38:08 -0800 Subject: [PATCH] fix broken internal metrics names and make them optional (#203) * improved internal metrics config setup * internal metric names are sanitized at runtime --- scope.go | 28 ++++++++----- scope_registry.go | 70 +++++++++++++++++++------------- scope_registry_test.go | 6 +-- scope_test.go | 92 +++++++++++++++++++++++++----------------- 4 files changed, 115 insertions(+), 81 deletions(-) diff --git a/scope.go b/scope.go index 7888fc48..074942c0 100644 --- a/scope.go +++ b/scope.go @@ -1,4 +1,4 @@ -// Copyright (c) 2022 Uber Technologies, Inc. +// Copyright (c) 2023 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,7 +28,13 @@ import ( "go.uber.org/atomic" ) +type InternalMetricOption int + const ( + Unset InternalMetricOption = iota + SendInternalMetrics + OmitInternalMetrics + _defaultInitialSliceSize = 16 ) @@ -95,15 +101,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 - registryShardCount uint - skipInternalMetrics bool + Tags map[string]string + Prefix string + Reporter StatsReporter + CachedReporter CachedStatsReporter + Separator string + DefaultBuckets Buckets + SanitizeOptions *SanitizeOptions + registryShardCount uint + internalMetricsOption InternalMetricOption } // NewRootScope creates a new root Scope with a set of options and @@ -173,7 +179,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.skipInternalMetrics) + s.registry = newScopeRegistryWithShardCount(s, opts.registryShardCount, opts.internalMetricsOption) if interval > 0 { s.wg.Add(1) diff --git a/scope_registry.go b/scope_registry.go index 37057a1d..2ab7e552 100644 --- a/scope_registry.go +++ b/scope_registry.go @@ -1,4 +1,4 @@ -// Copyright (c) 2022 Uber Technologies, Inc. +// Copyright (c) 2023 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 @@ -34,9 +34,9 @@ var ( // 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" ) type scopeRegistry struct { @@ -45,7 +45,7 @@ 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 // Toggles internal metrics reporting. - skipInternalMetrics bool + internalMetricsOption InternalMetricOption } type scopeBucket struct { @@ -53,16 +53,20 @@ type scopeBucket struct { s map[string]*scope } -func newScopeRegistryWithShardCount(root *scope, shardCount uint, skipInternalMetrics bool) *scopeRegistry { +func newScopeRegistryWithShardCount( + root *scope, + shardCount uint, + internalMetricsOption InternalMetricOption, +) *scopeRegistry { if shardCount == 0 { shardCount = uint(runtime.GOMAXPROCS(-1)) } r := &scopeRegistry{ - root: root, - subscopes: make([]*scopeBucket, shardCount), - seed: maphash.MakeSeed(), - skipInternalMetrics: skipInternalMetrics, + root: root, + subscopes: make([]*scopeBucket, shardCount), + seed: maphash.MakeSeed(), + internalMetricsOption: internalMetricsOption, } for i := uint(0); i < shardCount; i++ { r.subscopes[i] = &scopeBucket{ @@ -227,39 +231,47 @@ func (r *scopeRegistry) removeWithRLock(subscopeBucket *scopeBucket, key string) // Records internal Metrics' cardinalities. func (r *scopeRegistry) reportInternalMetrics() { - if r.skipInternalMetrics { + if r.internalMetricsOption != SendInternalMetrics { return } counters, gauges, histograms := atomic.Int64{}, atomic.Int64{}, atomic.Int64{} rootCounters, rootGauges, rootHistograms := atomic.Int64{}, atomic.Int64{}, atomic.Int64{} - r.ForEachScope(func(ss *scope) { - counterSliceLen, gaugeSliceLen, histogramSliceLen := int64(len(ss.countersSlice)), int64(len(ss.gaugesSlice)), int64(len(ss.histogramsSlice)) - if ss.root { // Root scope is referenced across all buckets. - rootCounters.Store(counterSliceLen) - rootGauges.Store(gaugeSliceLen) - rootHistograms.Store(histogramSliceLen) - return - } - counters.Add(counterSliceLen) - gauges.Add(gaugeSliceLen) - histograms.Add(histogramSliceLen) - }) + r.ForEachScope( + func(ss *scope) { + counterSliceLen, gaugeSliceLen, histogramSliceLen := int64(len(ss.countersSlice)), int64(len(ss.gaugesSlice)), int64(len(ss.histogramsSlice)) + if ss.root { // Root scope is referenced across all buckets. + rootCounters.Store(counterSliceLen) + rootGauges.Store(gaugeSliceLen) + rootHistograms.Store(histogramSliceLen) + return + } + counters.Add(counterSliceLen) + gauges.Add(gaugeSliceLen) + histograms.Add(histogramSliceLen) + }, + ) counters.Add(rootCounters.Load()) gauges.Add(rootGauges.Load()) histograms.Add(rootHistograms.Load()) if r.root.reporter != nil { - r.root.reporter.ReportCounter(counterCardinalityName, internalTags, counters.Load()) - r.root.reporter.ReportCounter(gaugeCardinalityName, internalTags, gauges.Load()) - r.root.reporter.ReportCounter(histogramCardinalityName, internalTags, histograms.Load()) + r.root.reporter.ReportCounter(r.root.sanitizer.Name(counterCardinalityName), internalTags, counters.Load()) + r.root.reporter.ReportCounter(r.root.sanitizer.Name(gaugeCardinalityName), internalTags, gauges.Load()) + r.root.reporter.ReportCounter(r.root.sanitizer.Name(histogramCardinalityName), internalTags, histograms.Load()) } if r.root.cachedReporter != nil { - numCounters := r.root.cachedReporter.AllocateCounter(counterCardinalityName, internalTags) - numGauges := r.root.cachedReporter.AllocateCounter(gaugeCardinalityName, internalTags) - numHistograms := r.root.cachedReporter.AllocateCounter(histogramCardinalityName, internalTags) + numCounters := r.root.cachedReporter.AllocateCounter( + r.root.sanitizer.Name(counterCardinalityName), internalTags, + ) + numGauges := r.root.cachedReporter.AllocateCounter( + r.root.sanitizer.Name(gaugeCardinalityName), internalTags, + ) + numHistograms := r.root.cachedReporter.AllocateCounter( + r.root.sanitizer.Name(histogramCardinalityName), internalTags, + ) numCounters.ReportCount(counters.Load()) numGauges.ReportCount(gauges.Load()) numHistograms.ReportCount(histograms.Load()) diff --git a/scope_registry_test.go b/scope_registry_test.go index c669a422..06951146 100644 --- a/scope_registry_test.go +++ b/scope_registry_test.go @@ -1,4 +1,4 @@ -// Copyright (c) 2022 Uber Technologies, Inc. +// Copyright (c) 2023 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 @@ -55,7 +55,7 @@ func TestVerifyCachedTaggedScopesAlloc(t *testing.T) { func TestNewTestStatsReporterOneScope(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: false}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: SendInternalMetrics}, 0) s := root.(*scope) numFakeCounters := 3 @@ -101,7 +101,7 @@ func TestNewTestStatsReporterOneScope(t *testing.T) { func TestNewTestStatsReporterManyScopes(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: false}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: SendInternalMetrics}, 0) wantCounters, wantGauges, wantHistograms := int64(3), int64(2), int64(1) s := root.(*scope) diff --git a/scope_test.go b/scope_test.go index 5d319458..992d3e36 100644 --- a/scope_test.go +++ b/scope_test.go @@ -1,4 +1,4 @@ -// Copyright (c) 2021 Uber Technologies, Inc. +// Copyright (c) 2023 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 @@ -353,7 +353,7 @@ func (r *testStatsReporter) Flush() { func TestWriteTimerImmediately(t *testing.T) { r := newTestStatsReporter() - s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) + s, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) defer closer.Close() r.tg.Add(1) s.Timer("ticky").Record(time.Millisecond * 175) @@ -362,7 +362,7 @@ func TestWriteTimerImmediately(t *testing.T) { func TestWriteTimerClosureImmediately(t *testing.T) { r := newTestStatsReporter() - s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) + s, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) defer closer.Close() r.tg.Add(1) tm := s.Timer("ticky") @@ -372,7 +372,7 @@ func TestWriteTimerClosureImmediately(t *testing.T) { func TestWriteReportLoop(t *testing.T) { r := newTestStatsReporter() - s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 10) + s, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 10) defer closer.Close() r.cg.Add(1) @@ -390,7 +390,7 @@ func TestWriteReportLoop(t *testing.T) { func TestCachedReportLoop(t *testing.T) { r := newTestStatsReporter() - s, closer := NewRootScope(ScopeOptions{CachedReporter: r, skipInternalMetrics: true}, 10) + s, closer := NewRootScope(ScopeOptions{CachedReporter: r, internalMetricsOption: OmitInternalMetrics}, 10) defer closer.Close() r.cg.Add(1) @@ -408,9 +408,9 @@ func TestCachedReportLoop(t *testing.T) { func testReportLoopFlushOnce(t *testing.T, cached bool) { r := newTestStatsReporter() - scopeOpts := ScopeOptions{CachedReporter: r, skipInternalMetrics: true} + scopeOpts := ScopeOptions{CachedReporter: r, internalMetricsOption: OmitInternalMetrics} if !cached { - scopeOpts = ScopeOptions{Reporter: r, skipInternalMetrics: true} + scopeOpts = ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics} } s, closer := NewRootScope(scopeOpts, 10*time.Minute) @@ -448,7 +448,7 @@ func TestReporterFlushOnce(t *testing.T) { func TestWriteOnce(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) defer closer.Close() s := root.(*scope) @@ -519,10 +519,10 @@ func TestHistogramSharedBucketMetrics(t *testing.T) { var ( r = newTestStatsReporter() scope = newRootScope(ScopeOptions{ - Prefix: "", - Tags: nil, - CachedReporter: r, - skipInternalMetrics: true, + Prefix: "", + Tags: nil, + CachedReporter: r, + internalMetricsOption: OmitInternalMetrics, }, 0) builder = func(s Scope) func(map[string]string) { buckets := MustMakeLinearValueBuckets(10, 10, 3) @@ -591,10 +591,10 @@ func TestConcurrentUpdates(t *testing.T) { counterIncrs = 5000 rs = newRootScope( ScopeOptions{ - Prefix: "", - Tags: nil, - CachedReporter: r, - skipInternalMetrics: true, + Prefix: "", + Tags: nil, + CachedReporter: r, + internalMetricsOption: OmitInternalMetrics, }, 0, ) scopes = []Scope{rs} @@ -640,9 +640,9 @@ func TestCounterSanitized(t *testing.T) { r := newTestStatsReporter() root, closer := NewRootScope(ScopeOptions{ - Reporter: r, - SanitizeOptions: &alphanumericSanitizerOpts, - skipInternalMetrics: true, + Reporter: r, + SanitizeOptions: &alphanumericSanitizerOpts, + internalMetricsOption: OmitInternalMetrics, }, 0) defer closer.Close() @@ -698,7 +698,7 @@ func TestCounterSanitized(t *testing.T) { func TestCachedReporter(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{CachedReporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope(ScopeOptions{CachedReporter: r, internalMetricsOption: OmitInternalMetrics}, 0) defer closer.Close() s := root.(*scope) @@ -735,7 +735,7 @@ func TestCachedReporter(t *testing.T) { func TestRootScopeWithoutPrefix(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) defer closer.Close() s := root.(*scope) @@ -769,7 +769,9 @@ func TestRootScopeWithoutPrefix(t *testing.T) { func TestRootScopeWithPrefix(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope( + ScopeOptions{Prefix: "foo", Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0, + ) defer closer.Close() s := root.(*scope) @@ -803,7 +805,11 @@ func TestRootScopeWithPrefix(t *testing.T) { func TestRootScopeWithDifferentSeparator(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Separator: "_", Reporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope( + ScopeOptions{ + Prefix: "foo", Separator: "_", Reporter: r, internalMetricsOption: OmitInternalMetrics, + }, 0, + ) defer closer.Close() s := root.(*scope) @@ -837,7 +843,9 @@ func TestRootScopeWithDifferentSeparator(t *testing.T) { func TestSubScope(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope( + ScopeOptions{Prefix: "foo", Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0, + ) defer closer.Close() tags := map[string]string{"foo": "bar"} @@ -879,7 +887,7 @@ func TestSubScope(t *testing.T) { func TestSubScopeClose(t *testing.T) { r := newTestStatsReporter() - rs, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, skipInternalMetrics: true}, 0) + rs, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) // defer closer.Close() _ = closer @@ -970,7 +978,11 @@ func TestTaggedSubScope(t *testing.T) { r := newTestStatsReporter() ts := map[string]string{"env": "test"} - root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Tags: ts, Reporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope( + ScopeOptions{ + Prefix: "foo", Tags: ts, Reporter: r, internalMetricsOption: OmitInternalMetrics, + }, 0, + ) defer closer.Close() s := root.(*scope) @@ -1022,11 +1034,11 @@ func TestTaggedSanitizedSubScope(t *testing.T) { ts := map[string]string{"env": "test:env"} root, closer := NewRootScope(ScopeOptions{ - Prefix: "foo", - Tags: ts, - Reporter: r, - SanitizeOptions: &alphanumericSanitizerOpts, - skipInternalMetrics: true, + Prefix: "foo", + Tags: ts, + Reporter: r, + SanitizeOptions: &alphanumericSanitizerOpts, + internalMetricsOption: OmitInternalMetrics, }, 0) defer closer.Close() s := root.(*scope) @@ -1055,7 +1067,11 @@ func TestTaggedExistingReturnsSameScope(t *testing.T) { nil, {"env": "test"}, } { - root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Tags: initialTags, Reporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope( + ScopeOptions{ + Prefix: "foo", Tags: initialTags, Reporter: r, internalMetricsOption: OmitInternalMetrics, + }, 0, + ) defer closer.Close() rootScope := root.(*scope) @@ -1132,7 +1148,7 @@ func TestSnapshot(t *testing.T) { func TestCapabilities(t *testing.T) { r := newTestStatsReporter() - s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) + s, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) defer closer.Close() assert.True(t, s.Capabilities().Reporting()) assert.False(t, s.Capabilities().Tagging()) @@ -1160,8 +1176,8 @@ func TestScopeDefaultBuckets(t *testing.T) { 90 * time.Millisecond, 120 * time.Millisecond, }, - Reporter: r, - skipInternalMetrics: true, + Reporter: r, + internalMetricsOption: OmitInternalMetrics, }, 0) defer closer.Close() @@ -1192,7 +1208,7 @@ func newTestMets(scope Scope) testMets { func TestReturnByValue(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) defer closer.Close() s := root.(*scope) @@ -1209,7 +1225,7 @@ func TestReturnByValue(t *testing.T) { func TestScopeAvoidReportLoopRunOnClose(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) s := root.(*scope) s.reportLoopRun() @@ -1224,7 +1240,7 @@ func TestScopeAvoidReportLoopRunOnClose(t *testing.T) { func TestScopeFlushOnClose(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, time.Hour) + root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, time.Hour) r.cg.Add(1) root.Counter("foo").Inc(1)