Skip to content

Add fixes against race conditions #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .github/workflows/run-tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
name: Run tests

on:
push:
branches: [ master ]
pull_request:
branches: [ master ]

jobs:

build:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v2
- uses: actions/setup-go@v2
with:
go-version: '^1.14'
- name: Test
run: make test
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
test:
go test -v -race ./...
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
[![Tests](https://github.com/penglongli/gin-metrics/actions/workflows/go.yml/badge.svg?branch=master)](https://github.com/penglongli/gin-metrics/actions/workflows/run-tests.yml)

# gin-metrics
gin-gonic/gin metrics exporter for Prometheus.

Expand Down
8 changes: 8 additions & 0 deletions bloom/bloom.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ package bloom

import (
"github.com/bits-and-blooms/bitset"
"sync"
)

const defaultSize = 2 << 24

var seeds = []uint{7, 11, 13, 31, 37, 61}

type BloomFilter struct {
mu sync.Mutex
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can use RWMutex, then bf.Contains can use bf.mu.RLock()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran some benchmarks, and these are the results:

  1. Current implementation (sync.Mutex):
goos: linux
goarch: amd64
pkg: github.com/penglongli/gin-metrics/ginmetrics
cpu: Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz
BenchmarkMiddlewareSamePort-4   	   10000	    187130 ns/op
BenchmarkMiddlewareSamePort-4   	   10000	    368713 ns/op
BenchmarkMiddlewareSamePort-4   	   10000	    376937 ns/op
BenchmarkMiddlewareSamePort-4   	   10000	    360904 ns/op
BenchmarkMiddlewareSamePort-4   	   10000	    178943 ns/op
PASS
ok  	github.com/penglongli/gin-metrics/ginmetrics	19.239s
  1. Suggesteg implementation (sync.RWMutex, where bf.Contains() is protected by bf.mu.RLock()):
goos: linux
goarch: amd64
pkg: github.com/penglongli/gin-metrics/ginmetrics
cpu: Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz
BenchmarkMiddlewareSamePort-4   	   10000	    179478 ns/op
BenchmarkMiddlewareSamePort-4   	   10000	    364757 ns/op
BenchmarkMiddlewareSamePort-4   	   10000	    365689 ns/op
BenchmarkMiddlewareSamePort-4   	   10000	    180693 ns/op
BenchmarkMiddlewareSamePort-4   	    9828	    360628 ns/op
PASS
ok  	github.com/penglongli/gin-metrics/ginmetrics	18.937s

Benchtest comparison:

name                  old time/op  new time/op  delta
MiddlewareSamePort-4   295µs ±39%   290µs ±38%   ~     (p=0.690 n=5+5)

As it can be seen the results are statistically indistinguishable. Increased the number of times benchmarks were run up to 25, and the result is still the same.

name                  old time/op  new time/op  delta
MiddlewareSamePort-4   278µs ±38%   264µs ±45%   ~     (p=0.658 n=25+25)

I suggest to keep it the way it is.

Set *bitset.BitSet
Funcs [6]simpleHash
}
Expand All @@ -23,12 +25,18 @@ func NewBloomFilter() *BloomFilter {
}

func (bf *BloomFilter) Add(value string) {
bf.mu.Lock()
defer bf.mu.Unlock()

for _, f := range bf.Funcs {
bf.Set.Set(f.hash(value))
}
}

func (bf *BloomFilter) Contains(value string) bool {
bf.mu.Lock()
defer bf.mu.Unlock()

if value == "" {
return false
}
Expand Down
109 changes: 109 additions & 0 deletions ginmetrics/middleware_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
package ginmetrics_test

import (
"fmt"
"net/http"
"net/http/httptest"
"sync"
"testing"

"github.com/gin-gonic/gin"

"github.com/penglongli/gin-metrics/ginmetrics"
)

func init() {
gin.SetMode(gin.ReleaseMode)
}

func TestMiddlewareSamePort(t *testing.T) {
r := newRouter()

ts := httptest.NewServer(r)
defer ts.Close()

var wg sync.WaitGroup
nLoops := 1000
wg.Add(nLoops)

for i := 0; i < nLoops; i++ {
go func(t *testing.T, i int) {
res, err := http.Get(ts.URL + fmt.Sprintf("/product/%d", i))
if err != nil {
t.Errorf("Expected nil, received %s", err.Error())
}

if res.StatusCode != http.StatusOK {
t.Errorf("Expected %d, received %d", http.StatusOK, res.StatusCode)
}
wg.Done()
}(t, i)
}

wg.Wait()
}

func TestMiddlewareDifferentPort(t *testing.T) {
appRouter, metricsRouter := newRouterSeparateMetrics()

ts := httptest.NewServer(appRouter)
tms := httptest.NewServer(metricsRouter)

defer ts.Close()
defer tms.Close()

var wg sync.WaitGroup
nLoops := 1000
wg.Add(nLoops)

for i := 0; i < nLoops; i++ {
go func(t *testing.T, i int) {
res, err := http.Get(ts.URL + fmt.Sprintf("/product/%d", i))
if err != nil {
t.Errorf("Expected nil, received %s", err.Error())
}

if res.StatusCode != http.StatusOK {
t.Errorf("Expected %d, received %d", http.StatusOK, res.StatusCode)
}
wg.Done()
}(t, i)
}

wg.Wait()
}

func newRouterSeparateMetrics() (*gin.Engine, *gin.Engine) {
appRouter := gin.New()
metricRouter := gin.Default()

m := ginmetrics.GetMonitor()
m.UseWithoutExposingEndpoint(appRouter)
m.Expose(metricRouter)

appRouter.GET("/product/:id", func(ctx *gin.Context) {
ctx.JSON(200, map[string]string{
"productId": ctx.Param("id"),
})
})

return appRouter, metricRouter
}

func newRouter() *gin.Engine {
r := gin.New()

m := ginmetrics.GetMonitor()
m.SetMetricPath("/metrics")
m.SetSlowTime(10)
m.SetDuration([]float64{0.1, 0.3, 1.2, 5, 10})
m.Use(r)

r.GET("/product/:id", func(ctx *gin.Context) {
ctx.JSON(200, map[string]string{
"productId": ctx.Param("id"),
})
})

return r
}
11 changes: 8 additions & 3 deletions ginmetrics/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package ginmetrics
import (
"github.com/pkg/errors"
"github.com/prometheus/client_golang/prometheus"

"sync"
)

type MetricType int
Expand All @@ -19,8 +21,10 @@ const (
)

var (
once sync.Once
monitor *Monitor

defaultDuration = []float64{0.1, 0.3, 1.2, 5, 10}
monitor *Monitor

promTypeHandler = map[MetricType]func(metric *Metric) error{
Counter: counterHandler,
Expand All @@ -41,14 +45,15 @@ type Monitor struct {
// GetMonitor used to get global Monitor object,
// this function returns a singleton object.
func GetMonitor() *Monitor {
if monitor == nil {
once.Do(func() {
monitor = &Monitor{
metricPath: defaultMetricPath,
slowTime: defaultSlowTime,
reqDuration: defaultDuration,
metrics: make(map[string]*Metric),
}
}
})

return monitor
}

Expand Down
20 changes: 20 additions & 0 deletions ginmetrics/types_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package ginmetrics

import (
"sync"
"testing"
)

func TestSingletonRacing(t *testing.T) {
var wg sync.WaitGroup
nLoops := 1000
wg.Add(nLoops)
for i := 0; i < nLoops; i++ {
go func() {
GetMonitor()
wg.Done()
}()
}

wg.Wait()
}