Skip to content

Commit

Permalink
tags: change serializeTags to never modify the tags map (#101)
Browse files Browse the repository at this point in the history 
This changes serializeTags() to not delete entries from the provided
tags map when keys/values are invalid. This prevents a race condition
when users pass the same tags map with invalid entries to
NewCounterWithTags, NewGaugeWithTags, or NewTimerWithTags.
  • Loading branch information
@charlievieth
charlievieth authored Aug 8, 2020
1 parent fc942d7 commit 00f2fa1
Show file tree
Hide file tree
Showing 4 changed files with 419 additions and 33 deletions.
55 changes: 27 additions & 28 deletions stats.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,29 +386,32 @@ func (s *statStore) newCounterWithTagSet(name string, tags tagSet) Counter {

var emptyPerInstanceTags = map[string]string{"_f": "i"}

// convertTagsToPerInstanceTagSet converts a tag map that is missing the
// per-instance key "_f" to a tagSet that has the per-instance key.
//
// Previously, we modified the map, but that is no longer allowed and creating
// a new tagSet is faster and more memory efficient than creating a duplicate
// map.
func convertTagsToPerInstanceTagSet(tags map[string]string) tagSet {
set := make(tagSet, 1, len(tags)+1)
set[0] = tagPair{key: "_f", value: "i"}
for k, v := range tags {
if k != "" && v != "" {
set = append(set, tagPair{key: k, value: replaceChars(v)})
}
}
set.Sort()
return set
}

func (s *statStore) NewPerInstanceCounter(name string, tags map[string]string) Counter {
if len(tags) == 0 {
return s.NewCounterWithTags(name, emptyPerInstanceTags)
}

if _, found := tags["_f"]; !found {
tags["_f"] = "i"
}

return s.NewCounterWithTags(name, tags)
}

type statCache struct {
cache sync.Map
new func() interface{}
}

func (s *statCache) LoadOrCreate(key string) interface{} {
if v, ok := s.cache.Load(key); ok {
return v
if _, found := tags["_f"]; found {
return s.NewCounterWithTags(name, tags)
}
v, _ := s.cache.LoadOrStore(key, s.new())
return v
return s.newCounterWithTagSet(name, convertTagsToPerInstanceTagSet(tags))
}

func (s *statStore) newGauge(serializedName string) *gauge {
Expand Down Expand Up @@ -438,12 +441,10 @@ func (s *statStore) NewPerInstanceGauge(name string, tags map[string]string) Gau
if len(tags) == 0 {
return s.NewGaugeWithTags(name, emptyPerInstanceTags)
}

if _, found := tags["_f"]; !found {
tags["_f"] = "i"
if _, found := tags["_f"]; found {
return s.NewGaugeWithTags(name, tags)
}

return s.NewGaugeWithTags(name, tags)
return s.newGaugeWithTagSet(name, convertTagsToPerInstanceTagSet(tags))
}

func (s *statStore) newTimer(serializedName string) *timer {
Expand Down Expand Up @@ -473,12 +474,10 @@ func (s *statStore) NewPerInstanceTimer(name string, tags map[string]string) Tim
if len(tags) == 0 {
return s.NewTimerWithTags(name, emptyPerInstanceTags)
}

if _, found := tags["_f"]; !found {
tags["_f"] = "i"
if _, found := tags["_f"]; found {
return s.NewTimerWithTags(name, tags)
}

return s.NewTimerWithTags(name, tags)
return s.newTimerWithTagSet(name, convertTagsToPerInstanceTagSet(tags))
}

type subScope struct {
Expand Down
254 changes: 254 additions & 0 deletions stats_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (
"sync"
"testing"
"time"

"github.com/lyft/gostats/mock"
)

// Ensure flushing and adding generators does not race
Expand Down Expand Up @@ -88,6 +90,230 @@ func TestNewSubScope(t *testing.T) {
}
}

// Test that we never modify the tags map that is passed in
func TestTagMapNotModified(t *testing.T) {
type TagMethod func(scope Scope, name string, tags map[string]string)

copyTags := func(tags map[string]string) map[string]string {
orig := make(map[string]string, len(tags))
for k, v := range tags {
orig[k] = v
}
return orig
}

scopeGenerators := map[string]func() Scope{
"statStore": func() Scope { return &statStore{} },
"subScope": func() Scope { return newSubScope(&statStore{}, "name", nil) },
}

methodTestCases := map[string]TagMethod{
"ScopeWithTags": func(scope Scope, name string, tags map[string]string) {
scope.ScopeWithTags(name, tags)
},
"NewCounterWithTags": func(scope Scope, name string, tags map[string]string) {
scope.NewCounterWithTags(name, tags)
},
"NewPerInstanceCounter": func(scope Scope, name string, tags map[string]string) {
scope.NewPerInstanceCounter(name, tags)
},
"NewGaugeWithTags": func(scope Scope, name string, tags map[string]string) {
scope.NewGaugeWithTags(name, tags)
},
"NewPerInstanceGauge": func(scope Scope, name string, tags map[string]string) {
scope.NewPerInstanceGauge(name, tags)
},
"NewTimerWithTags": func(scope Scope, name string, tags map[string]string) {
scope.NewTimerWithTags(name, tags)
},
"NewPerInstanceTimer": func(scope Scope, name string, tags map[string]string) {
scope.NewPerInstanceTimer(name, tags)
},
}

tagsTestCases := []map[string]string{
{}, // empty
{
"": "invalid_key",
},
{
"invalid_value": "",
},
{
"": "invalid_key",
"invalid_value": "",
},
{
"_f": "i",
},
{
"": "invalid_key",
"invalid_value": "",
"_f": "i",
},
{
"": "invalid_key",
"invalid_value": "",
"_f": "value",
"1": "1",
},
{
"": "invalid_key",
"invalid_value": "",
"1": "1",
"2": "2",
"3": "3",
},
}

for scopeName, newScope := range scopeGenerators {
for methodName, method := range methodTestCases {
t.Run(scopeName+"."+methodName, func(t *testing.T) {
for _, orig := range tagsTestCases {
tags := copyTags(orig)
method(newScope(), "test", tags)
if !reflect.DeepEqual(tags, orig) {
t.Errorf("modified input map: %+v want: %+v", tags, orig)
}
}
})
}

}
}

func TestPerInstanceStats(t *testing.T) {
testCases := []struct {
expected string
tags map[string]string
}{
{
expected: "name.___f=i",
tags: map[string]string{}, // empty
},
{
expected: "name.___f=i",
tags: map[string]string{
"": "invalid_key",
},
},
{
expected: "name.___f=i",
tags: map[string]string{
"invalid_value": "",
},
},
{
expected: "name.___f=i",
tags: map[string]string{
"_f": "i",
},
},
{
expected: "name.___f=xxx",
tags: map[string]string{
"_f": "xxx",
},
},
{
expected: "name.___f=xxx",
tags: map[string]string{
"": "invalid_key",
"_f": "xxx",
},
},
{
expected: "name.___f=xxx",
tags: map[string]string{
"invalid_value": "",
"_f": "xxx",
},
},
{
expected: "name.___f=xxx",
tags: map[string]string{
"invalid_value": "",
"": "invalid_key",
"_f": "xxx",
},
},
{
expected: "name.__1=1.___f=xxx",
tags: map[string]string{
"invalid_value": "",
"": "invalid_key",
"_f": "xxx",
"1": "1",
},
},
{
expected: "name.__1=1.___f=i",
tags: map[string]string{
"1": "1",
},
},
{
expected: "name.__1=1.__2=2.___f=i",
tags: map[string]string{
"1": "1",
"2": "2",
},
},
}

sink := mock.NewSink()

testPerInstanceMethods := func(t *testing.T, scope Scope) {
for _, x := range testCases {
sink.Reset()

scope.NewPerInstanceCounter("name", x.tags).Inc()
scope.Store().Flush()
for key := range sink.Counters() {
if key != x.expected {
t.Errorf("Counter (%+v): got: %q want: %q", x, key, x.expected)
}
break
}

scope.NewPerInstanceGauge("name", x.tags).Inc()
scope.Store().Flush()
for key := range sink.Counters() {
if key != x.expected {
t.Errorf("Gauge (%+v): got: %q want: %q", x, key, x.expected)
}
break
}

scope.NewPerInstanceTimer("name", x.tags).AddValue(1)
scope.Store().Flush()
for key := range sink.Counters() {
if key != x.expected {
t.Errorf("Timer (%+v): got: %q want: %q", x, key, x.expected)
}
break
}
}
}

t.Run("StatsStore", func(t *testing.T) {
store := &statStore{sink: sink}

testPerInstanceMethods(t, store)
})

t.Run("SubScope", func(t *testing.T) {
store := &subScope{registry: &statStore{sink: sink}, name: "x"}

// Add sub-scope prefix to the name
for i, x := range testCases {
testCases[i].expected = "x." + x.expected
}

testPerInstanceMethods(t, store)
})
}

func mergeTagsReference(s *subScope, tags map[string]string) tagSet {
a := make(tagSet, 0, len(tags))
for k, v := range tags {
Expand Down Expand Up @@ -411,3 +637,31 @@ func BenchmarkScopeMergeTags(b *testing.B) {
}
}
}

func BenchmarkStoreNewPerInstanceCounter(b *testing.B) {
b.Run("HasTag", func(b *testing.B) {
var store statStore
tags := map[string]string{
"1": "1",
"2": "2",
"3": "3",
"_f": "xxx",
}
for i := 0; i < b.N; i++ {
store.NewPerInstanceCounter("name", tags)
}
})

b.Run("MissingTag", func(b *testing.B) {
var store statStore
tags := map[string]string{
"1": "1",
"2": "2",
"3": "3",
"4": "4",
}
for i := 0; i < b.N; i++ {
store.NewPerInstanceCounter("name", tags)
}
})
}
Loading

0 comments on commit 00f2fa1

Please sign in to comment.