Skip to content

Commit

Permalink
Fix variable shadowing bug (honeycombio#519)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?

The tip of refinery was crashing in metrics with a nil pointer error. It was puzzling why a function that was supposed to never return nil was in fact returning nil. The problem was a subtle variable shadowing issue that I only found by writing a test script and running it in the debugger:

```go
	metric, ok = metrics[name]
	if !ok {
		// create new metric using create function and add to map
		metric := createMetric(name)
		metrics[name] = metric
	}
	lock.Unlock()
	return metric
```
The := on the createMetric created a new variable called `metric` that was scoped to the body of the if clause. The next line properly assigned it to the map and then threw it away and returned the nil pointer created on the top line.


## Short description of the changes

- Removed an unneeded `:`
- Added some tests for the getOrAdd function
- Fixed a couple of spelling errors and a linter nit
  • Loading branch information
kentquirk authored Sep 19, 2022
1 parent 19e3184 commit eb12764
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 5 deletions.
10 changes: 5 additions & 5 deletions metrics/honeycomb.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,9 @@ func (h *HoneycombMetrics) initLibhoney(mc config.HoneycombMetricsConfig) error
h.libhClient.AddDynamicField("memory_inuse", getAlloc)
startTime := time.Now()
h.libhClient.AddDynamicField("process_uptime_seconds", func() interface{} {
return time.Now().Sub(startTime) / time.Second
return time.Since(startTime) / time.Second
})
go h.reportToHoneycommb(ctx)
go h.reportToHoneycomb(ctx)
return nil
}

Expand Down Expand Up @@ -213,7 +213,7 @@ func (h *HoneycombMetrics) readMemStats(mem *runtime.MemStats) {
*mem = h.latestMemStats
}

func (h *HoneycombMetrics) reportToHoneycommb(ctx context.Context) {
func (h *HoneycombMetrics) reportToHoneycomb(ctx context.Context) {
tick := time.NewTicker(time.Duration(h.reportingFreq) * time.Second)
for {
select {
Expand Down Expand Up @@ -284,7 +284,7 @@ func (h *HoneycombMetrics) Register(name string, metricType string) {
case "histogram":
getOrAdd(&h.lock, name, h.histograms, createHistogram)
default:
h.Logger.Debug().Logf("unspported metric type %s", metricType)
h.Logger.Debug().Logf("unsupported metric type %s", metricType)
}
}

Expand All @@ -308,7 +308,7 @@ func getOrAdd[T *counter | *gauge | *histogram](lock *sync.RWMutex, name string,
metric, ok = metrics[name]
if !ok {
// create new metric using create function and add to map
metric := createMetric(name)
metric = createMetric(name)
metrics[name] = metric
}
lock.Unlock()
Expand Down
94 changes: 94 additions & 0 deletions metrics/honeycomb_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package metrics

import (
"sync"
"testing"

"github.com/stretchr/testify/assert"
)

// These tests do a concurrency check for the getOrAdd lock semantics, and generally verify that getOrAdd
// is functional under load.
func Test_getOrAdd_counter(t *testing.T) {
var lock *sync.RWMutex = &sync.RWMutex{}
var metrics map[string]*counter = make(map[string]*counter)

const nthreads = 5

wg := sync.WaitGroup{}

for i := 0; i < nthreads; i++ {
wg.Add(1)
go func() {
for j := 0; j < 1000; j++ {
name := "foo"
var ctr *counter = getOrAdd(lock, name, metrics, createCounter)
ctr.lock.Lock()
ctr.val++
ctr.lock.Unlock()
}
wg.Done()
}()
}
wg.Wait()

var ctr *counter = getOrAdd(lock, "foo", metrics, createCounter)
assert.Equal(t, nthreads*1000, ctr.val)
}

func Test_getOrAdd_gauge(t *testing.T) {
var lock *sync.RWMutex = &sync.RWMutex{}
var metrics map[string]*gauge = make(map[string]*gauge)

const nthreads = 5

wg := sync.WaitGroup{}

for i := 0; i < nthreads; i++ {
wg.Add(1)
go func() {
for j := 0; j < 1000; j++ {
name := "foo"
var g *gauge = getOrAdd(lock, name, metrics, createGauge)
g.lock.Lock()
g.val++
g.lock.Unlock()
}
wg.Done()
}()
}
wg.Wait()

var g *gauge = getOrAdd(lock, "foo", metrics, createGauge)
assert.Equal(t, float64(nthreads*1000), g.val)
}

func Test_getOrAdd_histogram(t *testing.T) {
var lock *sync.RWMutex = &sync.RWMutex{}
var metrics map[string]*histogram = make(map[string]*histogram)

const nthreads = 5

wg := sync.WaitGroup{}

for i := 0; i < nthreads; i++ {
wg.Add(1)
go func() {
for j := 0; j < 1000; j++ {
name := "foo"
var h *histogram = getOrAdd(lock, name, metrics, createHistogram)
h.lock.Lock()
if len(h.vals) == 0 {
h.vals = append(h.vals, 0)
}
h.vals[0]++
h.lock.Unlock()
}
wg.Done()
}()
}
wg.Wait()

var h *histogram = getOrAdd(lock, "foo", metrics, createHistogram)
assert.Equal(t, float64(nthreads*1000), h.vals[0])
}

0 comments on commit eb12764

Please sign in to comment.