From 9ad201e1d9a37d4cb01fc02afdcd0a312d096486 Mon Sep 17 00:00:00 2001 From: brawndou <112038567+brawndou@users.noreply.github.com> Date: Thu, 26 Jan 2023 16:59:03 -0800 Subject: [PATCH] Revert "fix broken internal metrics names and make them optional (#203)" This reverts commit 24b844bfdc627df021d0fee64b2d17a982224cc7. --- scope.go | 28 +++++-------- scope_registry.go | 70 +++++++++++++------------------- scope_registry_test.go | 6 +-- scope_test.go | 92 +++++++++++++++++------------------------- 4 files changed, 81 insertions(+), 115 deletions(-) diff --git a/scope.go b/scope.go index 074942c0..7888fc48 100644 --- a/scope.go +++ b/scope.go @@ -1,4 +1,4 @@ -// Copyright (c) 2023 Uber Technologies, Inc. +// Copyright (c) 2022 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,13 +28,7 @@ import ( "go.uber.org/atomic" ) -type InternalMetricOption int - const ( - Unset InternalMetricOption = iota - SendInternalMetrics - OmitInternalMetrics - _defaultInitialSliceSize = 16 ) @@ -101,15 +95,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 - internalMetricsOption InternalMetricOption + Tags map[string]string + Prefix string + Reporter StatsReporter + CachedReporter CachedStatsReporter + Separator string + DefaultBuckets Buckets + SanitizeOptions *SanitizeOptions + registryShardCount uint + skipInternalMetrics bool } // NewRootScope creates a new root Scope with a set of options and @@ -179,7 +173,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.internalMetricsOption) + s.registry = newScopeRegistryWithShardCount(s, opts.registryShardCount, opts.skipInternalMetrics) if interval > 0 { s.wg.Add(1) diff --git a/scope_registry.go b/scope_registry.go index 2ab7e552..37057a1d 100644 --- a/scope_registry.go +++ b/scope_registry.go @@ -1,4 +1,4 @@ -// Copyright (c) 2023 Uber Technologies, Inc. +// Copyright (c) 2022 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. - internalMetricsOption InternalMetricOption + skipInternalMetrics bool } type scopeBucket struct { @@ -53,20 +53,16 @@ type scopeBucket struct { s map[string]*scope } -func newScopeRegistryWithShardCount( - root *scope, - shardCount uint, - internalMetricsOption InternalMetricOption, -) *scopeRegistry { +func newScopeRegistryWithShardCount(root *scope, shardCount uint, skipInternalMetrics bool) *scopeRegistry { if shardCount == 0 { shardCount = uint(runtime.GOMAXPROCS(-1)) } r := &scopeRegistry{ - root: root, - subscopes: make([]*scopeBucket, shardCount), - seed: maphash.MakeSeed(), - internalMetricsOption: internalMetricsOption, + root: root, + subscopes: make([]*scopeBucket, shardCount), + seed: maphash.MakeSeed(), + skipInternalMetrics: skipInternalMetrics, } for i := uint(0); i < shardCount; i++ { r.subscopes[i] = &scopeBucket{ @@ -231,47 +227,39 @@ func (r *scopeRegistry) removeWithRLock(subscopeBucket *scopeBucket, key string) // Records internal Metrics' cardinalities. func (r *scopeRegistry) reportInternalMetrics() { - if r.internalMetricsOption != SendInternalMetrics { + if r.skipInternalMetrics { 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(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()) + r.root.reporter.ReportCounter(counterCardinalityName, internalTags, counters.Load()) + r.root.reporter.ReportCounter(gaugeCardinalityName, internalTags, gauges.Load()) + r.root.reporter.ReportCounter(histogramCardinalityName, internalTags, histograms.Load()) } if r.root.cachedReporter != nil { - 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 := r.root.cachedReporter.AllocateCounter(counterCardinalityName, internalTags) + numGauges := r.root.cachedReporter.AllocateCounter(gaugeCardinalityName, internalTags) + numHistograms := r.root.cachedReporter.AllocateCounter(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 06951146..c669a422 100644 --- a/scope_registry_test.go +++ b/scope_registry_test.go @@ -1,4 +1,4 @@ -// Copyright (c) 2023 Uber Technologies, Inc. +// Copyright (c) 2022 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, internalMetricsOption: SendInternalMetrics}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: false}, 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, internalMetricsOption: SendInternalMetrics}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: false}, 0) wantCounters, wantGauges, wantHistograms := int64(3), int64(2), int64(1) s := root.(*scope) diff --git a/scope_test.go b/scope_test.go index 992d3e36..5d319458 100644 --- a/scope_test.go +++ b/scope_test.go @@ -1,4 +1,4 @@ -// Copyright (c) 2023 Uber Technologies, Inc. +// Copyright (c) 2021 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, internalMetricsOption: OmitInternalMetrics}, 0) + s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 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, internalMetricsOption: OmitInternalMetrics}, 0) + s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 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, internalMetricsOption: OmitInternalMetrics}, 10) + s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 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, internalMetricsOption: OmitInternalMetrics}, 10) + s, closer := NewRootScope(ScopeOptions{CachedReporter: r, skipInternalMetrics: true}, 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, internalMetricsOption: OmitInternalMetrics} + scopeOpts := ScopeOptions{CachedReporter: r, skipInternalMetrics: true} if !cached { - scopeOpts = ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics} + scopeOpts = ScopeOptions{Reporter: r, skipInternalMetrics: true} } 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, internalMetricsOption: OmitInternalMetrics}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 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, - internalMetricsOption: OmitInternalMetrics, + Prefix: "", + Tags: nil, + CachedReporter: r, + skipInternalMetrics: true, }, 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, - internalMetricsOption: OmitInternalMetrics, + Prefix: "", + Tags: nil, + CachedReporter: r, + skipInternalMetrics: true, }, 0, ) scopes = []Scope{rs} @@ -640,9 +640,9 @@ func TestCounterSanitized(t *testing.T) { r := newTestStatsReporter() root, closer := NewRootScope(ScopeOptions{ - Reporter: r, - SanitizeOptions: &alphanumericSanitizerOpts, - internalMetricsOption: OmitInternalMetrics, + Reporter: r, + SanitizeOptions: &alphanumericSanitizerOpts, + skipInternalMetrics: true, }, 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, internalMetricsOption: OmitInternalMetrics}, 0) + root, closer := NewRootScope(ScopeOptions{CachedReporter: r, skipInternalMetrics: true}, 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, internalMetricsOption: OmitInternalMetrics}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) defer closer.Close() s := root.(*scope) @@ -769,9 +769,7 @@ func TestRootScopeWithoutPrefix(t *testing.T) { func TestRootScopeWithPrefix(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope( - ScopeOptions{Prefix: "foo", Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0, - ) + root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, skipInternalMetrics: true}, 0) defer closer.Close() s := root.(*scope) @@ -805,11 +803,7 @@ func TestRootScopeWithPrefix(t *testing.T) { func TestRootScopeWithDifferentSeparator(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope( - ScopeOptions{ - Prefix: "foo", Separator: "_", Reporter: r, internalMetricsOption: OmitInternalMetrics, - }, 0, - ) + root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Separator: "_", Reporter: r, skipInternalMetrics: true}, 0) defer closer.Close() s := root.(*scope) @@ -843,9 +837,7 @@ func TestRootScopeWithDifferentSeparator(t *testing.T) { func TestSubScope(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope( - ScopeOptions{Prefix: "foo", Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0, - ) + root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, skipInternalMetrics: true}, 0) defer closer.Close() tags := map[string]string{"foo": "bar"} @@ -887,7 +879,7 @@ func TestSubScope(t *testing.T) { func TestSubScopeClose(t *testing.T) { r := newTestStatsReporter() - rs, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) + rs, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, skipInternalMetrics: true}, 0) // defer closer.Close() _ = closer @@ -978,11 +970,7 @@ func TestTaggedSubScope(t *testing.T) { r := newTestStatsReporter() ts := map[string]string{"env": "test"} - root, closer := NewRootScope( - ScopeOptions{ - Prefix: "foo", Tags: ts, Reporter: r, internalMetricsOption: OmitInternalMetrics, - }, 0, - ) + root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Tags: ts, Reporter: r, skipInternalMetrics: true}, 0) defer closer.Close() s := root.(*scope) @@ -1034,11 +1022,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, - internalMetricsOption: OmitInternalMetrics, + Prefix: "foo", + Tags: ts, + Reporter: r, + SanitizeOptions: &alphanumericSanitizerOpts, + skipInternalMetrics: true, }, 0) defer closer.Close() s := root.(*scope) @@ -1067,11 +1055,7 @@ func TestTaggedExistingReturnsSameScope(t *testing.T) { nil, {"env": "test"}, } { - root, closer := NewRootScope( - ScopeOptions{ - Prefix: "foo", Tags: initialTags, Reporter: r, internalMetricsOption: OmitInternalMetrics, - }, 0, - ) + root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Tags: initialTags, Reporter: r, skipInternalMetrics: true}, 0) defer closer.Close() rootScope := root.(*scope) @@ -1148,7 +1132,7 @@ func TestSnapshot(t *testing.T) { func TestCapabilities(t *testing.T) { r := newTestStatsReporter() - s, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) + s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) defer closer.Close() assert.True(t, s.Capabilities().Reporting()) assert.False(t, s.Capabilities().Tagging()) @@ -1176,8 +1160,8 @@ func TestScopeDefaultBuckets(t *testing.T) { 90 * time.Millisecond, 120 * time.Millisecond, }, - Reporter: r, - internalMetricsOption: OmitInternalMetrics, + Reporter: r, + skipInternalMetrics: true, }, 0) defer closer.Close() @@ -1208,7 +1192,7 @@ func newTestMets(scope Scope) testMets { func TestReturnByValue(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) defer closer.Close() s := root.(*scope) @@ -1225,7 +1209,7 @@ func TestReturnByValue(t *testing.T) { func TestScopeAvoidReportLoopRunOnClose(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0) + root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0) s := root.(*scope) s.reportLoopRun() @@ -1240,7 +1224,7 @@ func TestScopeAvoidReportLoopRunOnClose(t *testing.T) { func TestScopeFlushOnClose(t *testing.T) { r := newTestStatsReporter() - root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, time.Hour) + root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, time.Hour) r.cg.Add(1) root.Counter("foo").Inc(1)