Skip to content

Commit

Permalink
Backport #221 and #222 to master branch (#243)
Browse files Browse the repository at this point in the history
* [bug] Ensure that parent scopes do not return closed subscopes (#221)

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

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

* lint fix

* move comment

* update go.mod and go.sum

* add license header

---------

Co-authored-by: Matt Way <[email protected]>
  • Loading branch information
brawndou and mway authored Jan 25, 2024
1 parent 2498a0d commit 83f9861
Show file tree
Hide file tree
Showing 8 changed files with 371 additions and 17 deletions.
26 changes: 26 additions & 0 deletions generate.go
Original file line number Diff line number Diff line change
@@ -0,0 +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"
)
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ go 1.15

require (
github.com/cactus/go-statsd-client/v5 v5.0.0
github.com/golang/mock v1.6.0
github.com/pkg/errors v0.9.1
github.com/prometheus/client_golang v1.11.0
github.com/prometheus/client_model v0.2.0
Expand Down
21 changes: 20 additions & 1 deletion go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ github.com/go-logfmt/logfmt v0.4.0/go.mod h1:3RMwSq7FuexP4Kalkev3ejPJsZTpXXBr9+V
github.com/go-logfmt/logfmt v0.5.0/go.mod h1:wCYkCAKZfumFQihp8CzCvQ3paCTfi41vtzG1KdI/P7A=
github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY=
github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
github.com/golang/mock v1.6.0 h1:ErTB+efbowRARo13NNdxyJji2egdxLGQhRaY+DUumQc=
github.com/golang/mock v1.6.0/go.mod h1:p6yTPP+5HYm5mzsMV8JkE6ZKdX+/wYM6Hr+LicevLPs=
github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
Expand Down Expand Up @@ -102,24 +104,31 @@ github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PK
github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
github.com/twmb/murmur3 v1.1.5 h1:i9OLS9fkuLzBXjt6dptlAEyk58fJsSTXbRg3SgVyqgk=
github.com/twmb/murmur3 v1.1.5/go.mod h1:Qq/R7NUyOfr65zD+6Q5IHKsJLwP7exErjN6lyyq3OSQ=
github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k=
go.uber.org/atomic v1.7.0 h1:ADUqmZGgLDDfbSL9ZmPxKTybcoEYHgpYfELNoN+7hsw=
go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
go.uber.org/goleak v1.2.1 h1:NBol2c7O1ZokfZ0LEU9K6Whx/KnwvepVetCUhtKja4A=
go.uber.org/goleak v1.2.1/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4=
golang.org/x/crypto v0.0.0-20180904163835-0709b304e793/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20181114220301-adae6a3d119a/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190613194153-d28f0bde5980/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
golang.org/x/net v0.0.0-20200625001655-4c5254603344/go.mod h1:/O7V0waA8r7cgGh81Ro3o1hOxt32SMVPicZroKQ2sZA=
golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM=
golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20181116152217-5ac8a444bdc5/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
Expand All @@ -129,14 +138,24 @@ golang.org/x/sys v0.0.0-20200106162015-b016eb3dc98e/go.mod h1:h1NjWce9XRLGQEsW7w
golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200615200032-f1bc736245b1/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200625212154-ddb9806d33ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210510120138-977fb7262007/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20210603081109-ebe580a85c40 h1:JWgyZ1qgdTaF3N3oxC+MdTV7qvEEgHo3otj+HB5CM7Q=
golang.org/x/sys v0.0.0-20210603081109-ebe580a85c40/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
google.golang.org/appengine v1.4.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7/EB5XEv4=
google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0=
Expand Down
12 changes: 10 additions & 2 deletions scope.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ type scope struct {
done chan struct{}
wg sync.WaitGroup
root bool
testScope bool
}

// ScopeOptions is a set of options to construct a scope.
Expand All @@ -103,9 +104,11 @@ type ScopeOptions struct {
Separator string
DefaultBuckets Buckets
SanitizeOptions *SanitizeOptions
registryShardCount uint
OmitCardinalityMetrics bool
CardinalityMetricsTags map[string]string

testScope bool
registryShardCount uint
}

// NewRootScope creates a new root Scope with a set of options and
Expand All @@ -129,7 +132,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 {
Expand Down Expand Up @@ -174,6 +181,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
Expand Down
60 changes: 47 additions & 13 deletions scope_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,23 +181,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 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
}

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
}
Expand Down Expand Up @@ -225,10 +258,11 @@ 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[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
}
Expand Down
144 changes: 144 additions & 0 deletions scope_registry_external_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
// 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"
"testing"
"time"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"
"github.com/uber-go/tally/v4"
"github.com/uber-go/tally/v4/tallymock"
"go.uber.org/atomic"
)

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()

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,
OmitCardinalityMetrics: true,
}, 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())
}
2 changes: 1 addition & 1 deletion scope_registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,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)
}

Expand Down
Loading

0 comments on commit 83f9861

Please sign in to comment.