From 880f28565f70db34b0d7e47a5d4d80c6a3bfceeb Mon Sep 17 00:00:00 2001 From: brawndou <112038567+brawndou@users.noreply.github.com> Date: Thu, 2 Mar 2023 13:37:51 -0800 Subject: [PATCH 1/9] Add goleak checking to tests (#214) (#215) * add goroutine leak checking in scope testing and fix goroutine leak --- scope_registry_test.go | 23 +++++++++++++++++++---- scope_test.go | 25 ++++++++++++++++++++++--- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/scope_registry_test.go b/scope_registry_test.go index c8685355..63bea579 100644 --- a/scope_registry_test.go +++ b/scope_registry_test.go @@ -142,11 +142,22 @@ func TestNewTestStatsReporterManyScopes(t *testing.T) { } func TestForEachScopeConcurrent(t *testing.T) { - root := newRootScope(ScopeOptions{Prefix: "", Tags: nil}, 0) + var ( + root = newRootScope(ScopeOptions{Prefix: "", Tags: nil}, 0) + quit = make(chan struct{}) + done = make(chan struct{}) + ) + go func() { + defer close(done) for { - hello := root.Tagged(map[string]string{"a": "b"}).Counter("hello") - hello.Inc(1) + select { + case <-quit: + return + default: + hello := root.Tagged(map[string]string{"a": "b"}).Counter("hello") + hello.Inc(1) + } } }() @@ -160,9 +171,13 @@ func TestForEachScopeConcurrent(t *testing.T) { c = ss.counters["hello"] } ss.cm.RUnlock() - }) + }, + ) if c != nil { + quit <- struct{}{} break } } + + <-done } diff --git a/scope_test.go b/scope_test.go index e725e29b..2f67e5f4 100644 --- a/scope_test.go +++ b/scope_test.go @@ -33,8 +33,13 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} + var ( // alphanumericSanitizerOpts is the options to create a sanitizer which uses // the alphanumeric SanitizeFn. @@ -1168,21 +1173,35 @@ func TestSnapshot(t *testing.T) { } func TestSnapshotConcurrent(t *testing.T) { - scope := NewTestScope("", nil) + var ( + scope = NewTestScope("", nil) + quit = make(chan struct{}) + done = make(chan struct{}) + ) + go func() { + defer close(done) for { - hello := scope.Tagged(map[string]string{"a": "b"}).Counter("hello") - hello.Inc(1) + select { + case <-quit: + return + default: + hello := scope.Tagged(map[string]string{"a": "b"}).Counter("hello") + hello.Inc(1) + } } }() var val CounterSnapshot for { val = scope.Snapshot().Counters()["hello+a=b"] if val != nil { + quit <- struct{}{} break } } require.NotNil(t, val) + + <-done } func TestCapabilities(t *testing.T) { From 0fca31eeb635e7f1c9b30234b621261670fa1ab1 Mon Sep 17 00:00:00 2001 From: Matt Way Date: Fri, 9 Jun 2023 20:35:25 -0400 Subject: [PATCH 2/9] [bug] Ensure that parent scopes do not return closed subscopes (#221) --- generate.go | 7 ++ glide.lock | 31 +++++--- glide.yaml | 1 + scope.go | 17 ++--- scope_registry.go | 59 +++++++++++---- scope_registry_external_test.go | 111 +++++++++++++++++++++++++++++ scope_registry_test.go | 6 +- tallymock/stats_reporter.go | 122 ++++++++++++++++++++++++++++++++ 8 files changed, 320 insertions(+), 34 deletions(-) create mode 100644 generate.go create mode 100644 scope_registry_external_test.go create mode 100644 tallymock/stats_reporter.go diff --git a/generate.go b/generate.go new file mode 100644 index 00000000..43d2efd5 --- /dev/null +++ b/generate.go @@ -0,0 +1,7 @@ +package tally + +import ( + _ "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 diff --git a/glide.lock b/glide.lock index 6041632f..2c28d35b 100644 --- a/glide.lock +++ b/glide.lock @@ -1,5 +1,5 @@ -hash: 23f5ede86d6a4b9970e0d5ae025de37ab77246e36dbe5dbae98be73722e4954e -updated: 2022-06-02T16:30:47.793742-04:00 +hash: 61a6b12902a915c40b2e5c3720803e9c71fbfac455cbe58e5c1fb9e9474518fe +updated: 2023-06-10T00:32:40.687313472Z imports: - name: github.com/beorn7/perks version: 4c0e84591b9aa9e6dcfdf3e020114cd81f89d5f9 @@ -9,13 +9,18 @@ imports: version: 138b925ccdf617776955904ba7759fce64406cec subpackages: - statsd +- name: github.com/golang/mock + version: 5b455625bd2c8ffbcc0de6a0873f864ba3820904 + subpackages: + - gomock + - mockgen/model - name: github.com/golang/protobuf - version: ae97035608a719c7a1c1c41bed0ae0744bdb0c6f + version: 5d5e8c018a13017f9d5b8bf4fad64aaa42a87308 subpackages: - proto - ptypes/timestamp - name: github.com/m3db/prometheus_client_golang - version: a457ed11e3d6f6b70bf47f9aa8a92e4136c941ff + version: 3601e01160ff9b74a616d661683e0ff884e5b0c5 subpackages: - prometheus - prometheus/internal @@ -26,7 +31,7 @@ imports: subpackages: - go - name: github.com/m3db/prometheus_common - version: bd06b6bd06817a628eae4783b5a7b8f85c6da410 + version: dfb62d9d4677e88e0cf818e9a4faab072eee2785 subpackages: - expfmt - internal/bitbucket.org/ww/goautoneg @@ -38,16 +43,20 @@ imports: - name: github.com/pkg/errors version: 614d223910a179a466c1767a985424175c39b465 - name: github.com/prometheus/procfs - version: f436cbb89ece38bf080d446b3ca27053b305eaac + version: 332e865adfebaa7eaedc94535a3f12f7e5eeb2d4 subpackages: - internal/fs - internal/util - name: github.com/twmb/murmur3 - version: ae8d9b870b19ddd1cae2d86cfff5278d931d94b8 + version: 3fcd9c20f2443ea2612fca5675f4cf6849e809b9 - name: go.uber.org/atomic - version: 3504dfaa1fa414923b1c8693f45d2f6931daf229 + version: 76f817c8b7e771cdffc2b9f11a7ebb80333ca92b +- name: golang.org/x/sys + version: b52f5441ce4ee981d7a9295a5ddad71e196486a7 + subpackages: + - unix - name: google.golang.org/protobuf - version: 32051b4f86e54c2142c7c05362c6e96ae3454a1c + version: f221882bfb484564f1714ae05f197dea2c76898d subpackages: - encoding/prototext - encoding/protowire @@ -107,6 +116,10 @@ testImports: subpackages: - assert - require +- name: go.uber.org/goleak + version: 4a14d384c51ba0c8e6a2fb4dc8bf83d15cdec096 + subpackages: + - internal/stack - name: golang.org/x/tools version: 3fe2afc9e626f32e91aff6eddb78b14743446865 subpackages: diff --git a/glide.yaml b/glide.yaml index 14ae9a88..aed0735e 100644 --- a/glide.yaml +++ b/glide.yaml @@ -24,6 +24,7 @@ import: version: ^1.27.0 - package: github.com/golang/protobuf version: ^1.5.2 +- package: github.com/golang/mock testImport: - package: github.com/axw/gocov version: 54b98cfcac0c63fb3f9bd8e7ad241b724d4e985b diff --git a/scope.go b/scope.go index 1418348d..211ddaee 100644 --- a/scope.go +++ b/scope.go @@ -105,15 +105,16 @@ 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 + Tags map[string]string + Prefix string + Reporter StatsReporter + CachedReporter CachedStatsReporter + Separator string + DefaultBuckets Buckets + SanitizeOptions *SanitizeOptions + MetricsOption InternalMetricOption + registryShardCount uint - MetricsOption InternalMetricOption } // NewRootScope creates a new root Scope with a set of options and diff --git a/scope_registry.go b/scope_registry.go index c52514db..71c43b19 100644 --- a/scope_registry.go +++ b/scope_registry.go @@ -152,23 +152,56 @@ func (r *scopeRegistry) Subscope(parent *scope, prefix string, tags map[string]s // as the memory layout of []byte is a superset of string the below casting is safe and does not do any alloc // However it cannot be used outside of the stack; a heap allocation is needed if that string needs to be stored // in the map as a key - if s, ok := r.lockedLookup(subscopeBucket, *(*string)(unsafe.Pointer(&buf))); ok { - subscopeBucket.mu.RUnlock() - return s + var ( + unsanitizedKey = *(*string)(unsafe.Pointer(&buf)) + sanitizedKey string + ) + + s, ok := r.lockedLookup(subscopeBucket, unsanitizedKey) + if ok { + // If this subscope isn't closed, return it. Otherwise, report it + // immediately and delete it so that a new (functional) scope can be + // returned instead. + if !s.closed.Load() { + subscopeBucket.mu.RUnlock() + return s + } + + switch { + case parent.reporter != nil: + s.report(parent.reporter) + case parent.cachedReporter != nil: + s.cachedReport() + } } - subscopeBucket.mu.RUnlock() - // heap allocating the buf as a string to keep the key in the subscopes map - preSanitizeKey := string(buf) tags = parent.copyAndSanitizeMap(tags) - key := scopeRegistryKey(prefix, parent.tags, tags) + sanitizedKey = scopeRegistryKey(prefix, parent.tags, tags) + + // If a scope was found above but we didn't return, we need to remove the + // scope from both keys. + if ok { + r.removeWithRLock(subscopeBucket, unsanitizedKey) + r.removeWithRLock(subscopeBucket, sanitizedKey) + s.clearMetrics() + } + + subscopeBucket.mu.RUnlock() + + // Force-allocate the unsafe string as a safe string. Note that neither + // string(x) nor x+"" will have the desired effect (the former is a nop, + // and the latter will likely be elided), so append a new character and + // truncate instead. + // + // ref: https://go.dev/play/p/sxhExUKSxCw + unsanitizedKey = (unsanitizedKey + ".")[:len(unsanitizedKey)] subscopeBucket.mu.Lock() defer subscopeBucket.mu.Unlock() - if s, ok := r.lockedLookup(subscopeBucket, key); ok { - if _, ok = r.lockedLookup(subscopeBucket, preSanitizeKey); !ok { - subscopeBucket.s[preSanitizeKey] = s + if s, ok := r.lockedLookup(subscopeBucket, sanitizedKey); ok { + if _, ok = r.lockedLookup(subscopeBucket, unsanitizedKey); !ok { + subscopeBucket.s[unsanitizedKey] = s } return s } @@ -197,9 +230,9 @@ func (r *scopeRegistry) Subscope(parent *scope, prefix string, tags map[string]s bucketCache: parent.bucketCache, done: make(chan struct{}), } - subscopeBucket.s[key] = subscope - if _, ok := r.lockedLookup(subscopeBucket, preSanitizeKey); !ok { - subscopeBucket.s[preSanitizeKey] = subscope + subscopeBucket.s[sanitizedKey] = subscope + if _, ok := r.lockedLookup(subscopeBucket, unsanitizedKey); !ok { + subscopeBucket.s[unsanitizedKey] = subscope } return subscope } diff --git a/scope_registry_external_test.go b/scope_registry_external_test.go new file mode 100644 index 00000000..820ee67b --- /dev/null +++ b/scope_registry_external_test.go @@ -0,0 +1,111 @@ +// 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 +// 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_test + +import ( + "io" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + "github.com/uber-go/tally/tallymock" +) + +func TestNoDefunctSubscopes(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + var ( + tags = map[string]string{ + "hello": "world", + } + mockreporter = tallymock.NewMockStatsReporter(ctrl) + ready = make(chan struct{}) + closed atomic.Bool + wg sync.WaitGroup + ) + wg.Add(2) + + mockreporter.EXPECT(). + ReportCounter("a", gomock.Any(), int64(123)). + Do(func(_ string, _ map[string]string, _ int64) { + wg.Done() + }). + Times(1) + mockreporter.EXPECT(). + ReportCounter("b", gomock.Any(), int64(456)). + Do(func(_ string, _ map[string]string, _ int64) { + wg.Done() + }). + Times(1) + + // Use flushing as a signal to determine if/when a closed scope + // would be removed from the registry's cache. + mockreporter.EXPECT(). + Flush(). + Do(func() { + // Don't unblock the ready channel until we've explicitly + // closed the scope. + if !closed.Load() { + return + } + + select { + case <-ready: + default: + close(ready) + } + }). + MinTimes(1) + + root, _ := tally.NewRootScope(tally.ScopeOptions{ + Reporter: mockreporter, + }, time.Millisecond) + + subscope := root.Tagged(tags) + requireClose(t, subscope) + subscope = root.Tagged(tags) + + // Signal and wait for the next flush to ensure that subscope can + // be a closed scope. + closed.Store(true) + <-ready + + // Use the maybe-closed subscope for counter A. + subscope.Counter("a").Inc(123) + + // Guarantee that counter B will not use a closed subscope. + subscope = root.Tagged(tags) + subscope.Counter("b").Inc(456) + + requireClose(t, root) + wg.Wait() +} + +func requireClose(t *testing.T, scope tally.Scope) { + x, ok := scope.(io.Closer) + require.True(t, ok) + require.NoError(t, x.Close()) +} diff --git a/scope_registry_test.go b/scope_registry_test.go index 63bea579..f04eb754 100644 --- a/scope_registry_test.go +++ b/scope_registry_test.go @@ -27,9 +27,7 @@ import ( "github.com/stretchr/testify/assert" ) -var ( - numInternalMetrics = 3 -) +var numInternalMetrics = 3 func TestVerifyCachedTaggedScopesAlloc(t *testing.T) { root, _ := NewRootScope(ScopeOptions{ @@ -49,7 +47,7 @@ func TestVerifyCachedTaggedScopesAlloc(t *testing.T) { "qux": "quux", }) }) - expected := 2.0 + expected := 3.0 assert.True(t, allocs <= expected, "the cached tagged scopes should allocate at most %.0f allocations, but did allocate %.0f", expected, allocs) } diff --git a/tallymock/stats_reporter.go b/tallymock/stats_reporter.go new file mode 100644 index 00000000..d3c1d449 --- /dev/null +++ b/tallymock/stats_reporter.go @@ -0,0 +1,122 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/uber-go/tally (interfaces: StatsReporter) + +// Package tallymock is a generated GoMock package. +package tallymock + +import ( + reflect "reflect" + time "time" + + gomock "github.com/golang/mock/gomock" + tally "github.com/uber-go/tally" +) + +// MockStatsReporter is a mock of StatsReporter interface. +type MockStatsReporter struct { + ctrl *gomock.Controller + recorder *MockStatsReporterMockRecorder +} + +// MockStatsReporterMockRecorder is the mock recorder for MockStatsReporter. +type MockStatsReporterMockRecorder struct { + mock *MockStatsReporter +} + +// NewMockStatsReporter creates a new mock instance. +func NewMockStatsReporter(ctrl *gomock.Controller) *MockStatsReporter { + mock := &MockStatsReporter{ctrl: ctrl} + mock.recorder = &MockStatsReporterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockStatsReporter) EXPECT() *MockStatsReporterMockRecorder { + return m.recorder +} + +// Capabilities mocks base method. +func (m *MockStatsReporter) Capabilities() tally.Capabilities { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Capabilities") + ret0, _ := ret[0].(tally.Capabilities) + return ret0 +} + +// Capabilities indicates an expected call of Capabilities. +func (mr *MockStatsReporterMockRecorder) Capabilities() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Capabilities", reflect.TypeOf((*MockStatsReporter)(nil).Capabilities)) +} + +// Flush mocks base method. +func (m *MockStatsReporter) Flush() { + m.ctrl.T.Helper() + m.ctrl.Call(m, "Flush") +} + +// Flush indicates an expected call of Flush. +func (mr *MockStatsReporterMockRecorder) Flush() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Flush", reflect.TypeOf((*MockStatsReporter)(nil).Flush)) +} + +// ReportCounter mocks base method. +func (m *MockStatsReporter) ReportCounter(arg0 string, arg1 map[string]string, arg2 int64) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "ReportCounter", arg0, arg1, arg2) +} + +// ReportCounter indicates an expected call of ReportCounter. +func (mr *MockStatsReporterMockRecorder) ReportCounter(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReportCounter", reflect.TypeOf((*MockStatsReporter)(nil).ReportCounter), arg0, arg1, arg2) +} + +// ReportGauge mocks base method. +func (m *MockStatsReporter) ReportGauge(arg0 string, arg1 map[string]string, arg2 float64) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "ReportGauge", arg0, arg1, arg2) +} + +// ReportGauge indicates an expected call of ReportGauge. +func (mr *MockStatsReporterMockRecorder) ReportGauge(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReportGauge", reflect.TypeOf((*MockStatsReporter)(nil).ReportGauge), arg0, arg1, arg2) +} + +// ReportHistogramDurationSamples mocks base method. +func (m *MockStatsReporter) ReportHistogramDurationSamples(arg0 string, arg1 map[string]string, arg2 tally.Buckets, arg3, arg4 time.Duration, arg5 int64) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "ReportHistogramDurationSamples", arg0, arg1, arg2, arg3, arg4, arg5) +} + +// ReportHistogramDurationSamples indicates an expected call of ReportHistogramDurationSamples. +func (mr *MockStatsReporterMockRecorder) ReportHistogramDurationSamples(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReportHistogramDurationSamples", reflect.TypeOf((*MockStatsReporter)(nil).ReportHistogramDurationSamples), arg0, arg1, arg2, arg3, arg4, arg5) +} + +// ReportHistogramValueSamples mocks base method. +func (m *MockStatsReporter) ReportHistogramValueSamples(arg0 string, arg1 map[string]string, arg2 tally.Buckets, arg3, arg4 float64, arg5 int64) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "ReportHistogramValueSamples", arg0, arg1, arg2, arg3, arg4, arg5) +} + +// ReportHistogramValueSamples indicates an expected call of ReportHistogramValueSamples. +func (mr *MockStatsReporterMockRecorder) ReportHistogramValueSamples(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReportHistogramValueSamples", reflect.TypeOf((*MockStatsReporter)(nil).ReportHistogramValueSamples), arg0, arg1, arg2, arg3, arg4, arg5) +} + +// ReportTimer mocks base method. +func (m *MockStatsReporter) ReportTimer(arg0 string, arg1 map[string]string, arg2 time.Duration) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "ReportTimer", arg0, arg1, arg2) +} + +// ReportTimer indicates an expected call of ReportTimer. +func (mr *MockStatsReporterMockRecorder) ReportTimer(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReportTimer", reflect.TypeOf((*MockStatsReporter)(nil).ReportTimer), arg0, arg1, arg2) +} From b9d43faacfc3ddba44c2e107d9375a7e4ffe4b39 Mon Sep 17 00:00:00 2001 From: Matt Way Date: Sat, 10 Jun 2023 20:19:23 -0400 Subject: [PATCH 3/9] TestScope: don't prune from registry when closed (#222) --- scope.go | 9 ++++++++- scope_registry.go | 9 +++++---- scope_registry_external_test.go | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/scope.go b/scope.go index 211ddaee..0a540758 100644 --- a/scope.go +++ b/scope.go @@ -101,6 +101,7 @@ type scope struct { done chan struct{} wg sync.WaitGroup root bool + testScope bool } // ScopeOptions is a set of options to construct a scope. @@ -114,6 +115,7 @@ type ScopeOptions struct { SanitizeOptions *SanitizeOptions MetricsOption InternalMetricOption + testScope bool registryShardCount uint } @@ -132,7 +134,11 @@ func NewTestScope( prefix string, tags map[string]string, ) TestScope { - return newRootScope(ScopeOptions{Prefix: prefix, Tags: tags}, 0) + return newRootScope(ScopeOptions{ + Prefix: prefix, + Tags: tags, + testScope: true, + }, 0) } func newRootScope(opts ScopeOptions, interval time.Duration) *scope { @@ -177,6 +183,7 @@ func newRootScope(opts ScopeOptions, interval time.Duration) *scope { separator: sanitizer.Name(opts.Separator), timers: make(map[string]*timer), root: true, + testScope: opts.testScope, } // NB(r): Take a copy of the tags on creation diff --git a/scope_registry.go b/scope_registry.go index 71c43b19..2996a14f 100644 --- a/scope_registry.go +++ b/scope_registry.go @@ -159,10 +159,10 @@ func (r *scopeRegistry) Subscope(parent *scope, prefix string, tags map[string]s s, ok := r.lockedLookup(subscopeBucket, unsanitizedKey) if ok { - // If this subscope isn't closed, return it. Otherwise, report it - // immediately and delete it so that a new (functional) scope can be - // returned instead. - if !s.closed.Load() { + // If this subscope isn't closed or is a test scope, return it. + // Otherwise, report it immediately and delete it so that a new + // (functional) scope can be returned instead. + if !s.closed.Load() || s.testScope { subscopeBucket.mu.RUnlock() return s } @@ -229,6 +229,7 @@ func (r *scopeRegistry) Subscope(parent *scope, prefix string, tags map[string]s timers: make(map[string]*timer), bucketCache: parent.bucketCache, done: make(chan struct{}), + testScope: parent.testScope, } subscopeBucket.s[sanitizedKey] = subscope if _, ok := r.lockedLookup(subscopeBucket, unsanitizedKey); !ok { diff --git a/scope_registry_external_test.go b/scope_registry_external_test.go index 820ee67b..ea945bd5 100644 --- a/scope_registry_external_test.go +++ b/scope_registry_external_test.go @@ -33,6 +33,38 @@ import ( "github.com/uber-go/tally/tallymock" ) +func TestTestScopesNotPruned(t *testing.T) { + var ( + root = tally.NewTestScope("", nil) + subscope = root.SubScope("foo") + counter = subscope.Counter("bar") + ) + + counter.Inc(123) + + closer, ok := subscope.(io.Closer) + require.True(t, ok) + require.NoError(t, closer.Close()) + + subscope = root.SubScope("foo") + counter = subscope.Counter("bar") + counter.Inc(123) + + var ( + snapshot = root.Snapshot() + counters = snapshot.Counters() + ) + require.Len(t, counters, 1) + require.Len(t, snapshot.Gauges(), 0) + require.Len(t, snapshot.Timers(), 0) + require.Len(t, snapshot.Histograms(), 0) + + val, ok := counters["foo.bar+"] + require.True(t, ok) + require.Equal(t, "foo.bar", val.Name()) + require.EqualValues(t, 246, val.Value()) +} + func TestNoDefunctSubscopes(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() From 58ed96f6049e5cd6f3f5566824e88ebd76a2d72f Mon Sep 17 00:00:00 2001 From: Matt Way Date: Sun, 11 Jun 2023 00:20:29 +0000 Subject: [PATCH 4/9] Prepare v3.5.5 --- version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.go b/version.go index ac3c9724..7da0e1b6 100644 --- a/version.go +++ b/version.go @@ -21,4 +21,4 @@ package tally // Version is the current version of the library. -const Version = "3.5.3" +const Version = "3.5.5" From 3853e468e7f358d7ba551e661c4cc8f51c74197e Mon Sep 17 00:00:00 2001 From: shaan420 Date: Fri, 6 Oct 2023 07:54:00 -0700 Subject: [PATCH 5/9] Prepare v3.5.6 (#232) * Set default reporting interval (#226) * Enforce minimum reporting interval * Add new RootScope constructor with default interval * Test with go >= 1.18 (#228) * add custom tags to internal metrics (#231) * add custom tags to internal metrics * make unit test stricter, update version * update version to v3.5.6 --------- Co-authored-by: Vytenis Darulis --- .travis.yml | 10 +++----- m3/config.go | 5 ++++ m3/config_test.go | 1 + m3/reporter.go | 6 +++++ m3/reporter_test.go | 59 +++++++++++++++++++++++++++++++++++++++++++++ scope.go | 9 ++++++- scope_test.go | 20 +++++++++++++++ version.go | 2 +- 8 files changed, 104 insertions(+), 8 deletions(-) diff --git a/.travis.yml b/.travis.yml index 990e93bd..c43d4211 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,12 +1,10 @@ language: go sudo: false go: - - 1.14.x - - 1.15.x - - 1.16.x -env: - global: - - GO15VENDOREXPERIMENT=1 + - 1.18.x + - 1.19.x + - 1.20.x + - 1.21.x cache: directories: - vendor diff --git a/m3/config.go b/m3/config.go index cbeb4b6f..ccfb51ce 100644 --- a/m3/config.go +++ b/m3/config.go @@ -49,6 +49,10 @@ type Configuration struct { // HistogramBucketTagPrecision is precision to use when formatting the metric tag // with the histogram bucket bound values. HistogramBucketTagPrecision uint `yaml:"histogramBucketTagPrecision"` + + // CommonTagsInternal are tags that should be added to all internal metrics + // emitted by the reporter. + CommonTagsInternal map[string]string `yaml:"commonTagsInternal"` } // NewReporter creates a new M3 reporter from this configuration. @@ -66,5 +70,6 @@ func (c Configuration) NewReporter() (Reporter, error) { MaxPacketSizeBytes: c.PacketSize, IncludeHost: c.IncludeHost, HistogramBucketTagPrecision: c.HistogramBucketTagPrecision, + InternalTags: c.CommonTagsInternal, }) } diff --git a/m3/config_test.go b/m3/config_test.go index 7266c9e9..1eb7726a 100644 --- a/m3/config_test.go +++ b/m3/config_test.go @@ -43,6 +43,7 @@ func TestConfigSimple(t *testing.T) { assert.True(t, ok) assert.True(t, tagEquals(reporter.commonTags, "service", "my-service")) assert.True(t, tagEquals(reporter.commonTags, "env", "test")) + assert.Equal(t, 0, len(c.CommonTagsInternal)) } func TestConfigMulti(t *testing.T) { diff --git a/m3/reporter.go b/m3/reporter.go index 72d0fd02..6ba69308 100644 --- a/m3/reporter.go +++ b/m3/reporter.go @@ -149,6 +149,7 @@ type Options struct { HistogramBucketIDName string HistogramBucketName string HistogramBucketTagPrecision uint + InternalTags map[string]string } // NewReporter creates a new M3 reporter. @@ -288,6 +289,11 @@ func NewReporter(opts Options) (Reporter, error) { internalTags := map[string]string{ "version": tally.Version, } + + for k, v := range opts.InternalTags { + internalTags[k] = v + } + r.batchSizeHistogram = r.AllocateHistogram("tally.internal.batch-size", internalTags, buckets) r.numBatchesCounter = r.AllocateCounter("tally.internal.num-batches", internalTags) r.numMetricsCounter = r.AllocateCounter("tally.internal.num-metrics", internalTags) diff --git a/m3/reporter_test.go b/m3/reporter_test.go index ea7b4f9f..98c87936 100644 --- a/m3/reporter_test.go +++ b/m3/reporter_test.go @@ -559,6 +559,58 @@ func TestReporterResetTagsAfterReturnToPool(t *testing.T) { require.Equal(t, 0, len(filtered[1].GetTags())) } +func TestReporterCommmonTagsInternal(t *testing.T) { + var wg sync.WaitGroup + server := newFakeM3Server(t, &wg, false, Compact) + go server.Serve() + defer server.Close() + + internalTags := map[string]string{ + "internal1": "test1", + "internal2": "test2", + } + + r, err := NewReporter(Options{ + HostPorts: []string{server.Addr}, + Service: "test-service", + CommonTags: defaultCommonTags, + MaxQueueSize: queueSize, + IncludeHost: true, + MaxPacketSizeBytes: maxPacketSize, + InternalTags: internalTags, + }) + require.NoError(t, err) + defer r.Close() + + c := r.AllocateCounter("testCounter1", nil) + c.ReportCount(1) + wg.Add(internalMetrics + 1) + r.Flush() + wg.Wait() + + numInternalMetricsActual := 0 + metrics := server.Service.getMetrics() + require.Equal(t, internalMetrics+1, len(metrics)) + for _, metric := range metrics { + if strings.HasPrefix(metric.Name, "tally.internal") { + numInternalMetricsActual++ + for k, v := range internalTags { + require.True(t, tagEquals(metric.Tags, k, v)) + } + } else { + require.Equal(t, "testCounter1", metric.Name) + require.False(t, tagIncluded(metric.Tags, "internal1")) + require.False(t, tagIncluded(metric.Tags, "internal2")) + } + // The following tags should not be present as part of the individual metrics + // as they are common tags. + require.False(t, tagIncluded(metric.Tags, "host")) + require.False(t, tagIncluded(metric.Tags, "instance")) + require.False(t, tagIncluded(metric.Tags, "service")) + } + require.Equal(t, internalMetrics, numInternalMetricsActual) +} + func TestReporterHasReportingAndTaggingCapability(t *testing.T) { r, err := NewReporter(Options{ HostPorts: []string{"127.0.0.1:9052"}, @@ -587,6 +639,13 @@ type fakeM3ServerPackets struct { values [][]byte } +// newFakeM3Server creates a new fake M3 server that listens on a random port +// and returns the server. +// The server will wait for the given wait group to be done before returning. +// If countBatches is true, the server will wait consider the wg.Add()s to be +// representing batches and will do a eg.Done() for each encountered batch. +// But if countBatches is false, the server will do the same thing but for individual +// metrics instead of batches. func newFakeM3Server(t *testing.T, wg *sync.WaitGroup, countBatches bool, protocol Protocol) *fakeM3Server { service := newFakeM3Service(wg, countBatches) processor := m3thrift.NewM3Processor(service) diff --git a/scope.go b/scope.go index 0a540758..7a645bfd 100644 --- a/scope.go +++ b/scope.go @@ -39,7 +39,8 @@ const ( // OmitInternalMetrics turns off internal metrics submission. OmitInternalMetrics - _defaultInitialSliceSize = 16 + _defaultInitialSliceSize = 16 + _defaultReportingInterval = 2 * time.Second ) var ( @@ -127,6 +128,12 @@ func NewRootScope(opts ScopeOptions, interval time.Duration) (Scope, io.Closer) return s, s } +// NewRootScopeWithDefaultInterval invokes NewRootScope with the default +// reporting interval of 2s. +func NewRootScopeWithDefaultInterval(opts ScopeOptions) (Scope, io.Closer) { + return NewRootScope(opts, _defaultReportingInterval) +} + // NewTestScope creates a new Scope without a stats reporter with the // given prefix and adds the ability to take snapshots of metrics emitted // to it. diff --git a/scope_test.go b/scope_test.go index 2f67e5f4..0e6820a4 100644 --- a/scope_test.go +++ b/scope_test.go @@ -397,6 +397,26 @@ func TestWriteReportLoop(t *testing.T) { r.WaitAll() } +func TestWriteReportLoopDefaultInterval(t *testing.T) { + r := newTestStatsReporter() + s, closer := NewRootScopeWithDefaultInterval( + ScopeOptions{Reporter: r, MetricsOption: OmitInternalMetrics}, + ) + defer closer.Close() + + r.cg.Add(1) + s.Counter("bar").Inc(1) + r.gg.Add(1) + s.Gauge("zed").Update(1) + r.tg.Add(1) + s.Timer("ticky").Record(time.Millisecond * 175) + r.hg.Add(1) + s.Histogram("baz", MustMakeLinearValueBuckets(0, 10, 10)). + RecordValue(42.42) + + r.WaitAll() +} + func TestCachedReportLoop(t *testing.T) { r := newTestStatsReporter() s, closer := NewRootScope(ScopeOptions{CachedReporter: r, MetricsOption: OmitInternalMetrics}, 10) diff --git a/version.go b/version.go index 7da0e1b6..1320eb7f 100644 --- a/version.go +++ b/version.go @@ -21,4 +21,4 @@ package tally // Version is the current version of the library. -const Version = "3.5.5" +const Version = "3.5.6" From 15cb1ff707607a7dce63ebd05499ad91ffc2b80f Mon Sep 17 00:00:00 2001 From: shaan420 Date: Fri, 6 Oct 2023 19:43:47 -0700 Subject: [PATCH 6/9] rename CommonTagsInternal to InternalTags (#233) * rename CommonTagsInternal to InternalTags --- m3/config.go | 6 +++--- m3/config_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/m3/config.go b/m3/config.go index ccfb51ce..0e9ad577 100644 --- a/m3/config.go +++ b/m3/config.go @@ -50,9 +50,9 @@ type Configuration struct { // with the histogram bucket bound values. HistogramBucketTagPrecision uint `yaml:"histogramBucketTagPrecision"` - // CommonTagsInternal are tags that should be added to all internal metrics + // InternalTags are tags that should be added to all internal metrics // emitted by the reporter. - CommonTagsInternal map[string]string `yaml:"commonTagsInternal"` + InternalTags map[string]string `yaml:"internalTags"` } // NewReporter creates a new M3 reporter from this configuration. @@ -70,6 +70,6 @@ func (c Configuration) NewReporter() (Reporter, error) { MaxPacketSizeBytes: c.PacketSize, IncludeHost: c.IncludeHost, HistogramBucketTagPrecision: c.HistogramBucketTagPrecision, - InternalTags: c.CommonTagsInternal, + InternalTags: c.InternalTags, }) } diff --git a/m3/config_test.go b/m3/config_test.go index 1eb7726a..cb154efc 100644 --- a/m3/config_test.go +++ b/m3/config_test.go @@ -43,7 +43,7 @@ func TestConfigSimple(t *testing.T) { assert.True(t, ok) assert.True(t, tagEquals(reporter.commonTags, "service", "my-service")) assert.True(t, tagEquals(reporter.commonTags, "env", "test")) - assert.Equal(t, 0, len(c.CommonTagsInternal)) + assert.Equal(t, 0, len(c.InternalTags)) } func TestConfigMulti(t *testing.T) { From 35188768e73b151aed11056a8ca30bccf8c5482d Mon Sep 17 00:00:00 2001 From: shaan420 Date: Tue, 10 Oct 2023 16:03:46 +0530 Subject: [PATCH 7/9] update version to 3.5.7 (#234) --- version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.go b/version.go index 1320eb7f..e1fe52d1 100644 --- a/version.go +++ b/version.go @@ -21,4 +21,4 @@ package tally // Version is the current version of the library. -const Version = "3.5.6" +const Version = "3.5.7" From 4c7b3bcdd544b02f98966458cecb01c6a426a034 Mon Sep 17 00:00:00 2001 From: shaan420 Date: Fri, 27 Oct 2023 21:53:41 +0530 Subject: [PATCH 8/9] redact host and instance tags for internal metrics (#237) (#238) * redact host and instance tags for internal metrics * fix unit test --- m3/reporter.go | 6 +++++- m3/reporter_test.go | 7 +++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/m3/reporter.go b/m3/reporter.go index 6ba69308..7cde88cf 100644 --- a/m3/reporter.go +++ b/m3/reporter.go @@ -66,6 +66,8 @@ 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. @@ -287,7 +289,9 @@ func NewReporter(opts Options) (Reporter, error) { } internalTags := map[string]string{ - "version": tally.Version, + "version": tally.Version, + "host": DefaultTagRedactValue, + "instance": DefaultTagRedactValue, } for k, v := range opts.InternalTags { diff --git a/m3/reporter_test.go b/m3/reporter_test.go index 98c87936..eab0d17d 100644 --- a/m3/reporter_test.go +++ b/m3/reporter_test.go @@ -597,15 +597,18 @@ func TestReporterCommmonTagsInternal(t *testing.T) { for k, v := range internalTags { require.True(t, tagEquals(metric.Tags, k, v)) } + + // The following tags should be redacted. + require.True(t, tagEquals(metric.Tags, "host", DefaultTagRedactValue)) + require.True(t, tagEquals(metric.Tags, "instance", DefaultTagRedactValue)) } else { require.Equal(t, "testCounter1", metric.Name) require.False(t, tagIncluded(metric.Tags, "internal1")) require.False(t, tagIncluded(metric.Tags, "internal2")) } + // The following tags should not be present as part of the individual metrics // as they are common tags. - require.False(t, tagIncluded(metric.Tags, "host")) - require.False(t, tagIncluded(metric.Tags, "instance")) require.False(t, tagIncluded(metric.Tags, "service")) } require.Equal(t, internalMetrics, numInternalMetricsActual) From 5d0c8e8d7d1be0c8ce150e69d5f4274b02bbf494 Mon Sep 17 00:00:00 2001 From: shaan420 Date: Fri, 27 Oct 2023 21:56:20 +0530 Subject: [PATCH 9/9] update version to 3.5.8 (#239) --- version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.go b/version.go index e1fe52d1..715d8dc9 100644 --- a/version.go +++ b/version.go @@ -21,4 +21,4 @@ package tally // Version is the current version of the library. -const Version = "3.5.7" +const Version = "3.5.8"