SkyAPM / go2sky

Distributed tracing and monitor SDK in Go for Apache SkyWalking APM
https://skywalking.apache.org/
Apache License 2.0
448 stars 122 forks source link

fix(sampler): fixed random sampler called IsSampled will panic when concurrency call (#140) #142

Closed Luckyboys closed 2 years ago

Luckyboys commented 2 years ago

The rand.Rand is non-concurrency safe randomizer. If need concurrency safe, it need ensure the rand.Rand entity is using in one goroutine at the same time. In different solution research and benchmark it, we found use sync.Pool to avoid the rand.Rand entity using conflic.

codecov-commenter commented 2 years ago

Codecov Report

Merging #142 (f36b3d4) into master (343dbb7) will increase coverage by 0.56%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   69.82%   70.38%   +0.56%     
==========================================
  Files          17       17              
  Lines         898      915      +17     
==========================================
+ Hits          627      644      +17     
  Misses        224      224              
  Partials       47       47              
Impacted Files Coverage Δ
sampler.go 78.68% <100.00%> (+8.23%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 343dbb7...f36b3d4. Read the comment docs.

Luckyboys commented 2 years ago

@arugal @wu-sheng Please review this fix, thank you~

hanahmily commented 2 years ago

@Luckyboys Would you please submit the benchmark result? thank you

Luckyboys commented 2 years ago

@Luckyboys Would you please submit the benchmark result? thank you

Here is discussing in https://github.com/SkyAPM/go2sky/issues/140 benchmark results

CPU: Apple silicon M1 Pro 10 cores
Memory: 16GB
goos: darwin
goarch: arm64
pkg: github.com/SkyAPM/go2sky
BenchmarkRandomSampler_IsSampled-10                 195834091            5.965 ns/op
BenchmarkRandomMutexSampler_IsSampled-10             8900761           144.6 ns/op
BenchmarkRandomGlobalSampler_IsSampled-10            8900469           146.2 ns/op
BenchmarkRandomChannelSampler0_IsSampled-10          2612180           442.4 ns/op
BenchmarkRandomChannelSampler1_IsSampled-10          2770339           386.1 ns/op
BenchmarkRandomChannelSampler128_IsSampled-10        4160923           296.5 ns/op
BenchmarkRandomChannelSampler1024_IsSampled-10       4435966           293.8 ns/op
BenchmarkRandomPoolSampler_IsSampled-10             190921699            6.359 ns/op
PASS
ok      github.com/SkyAPM/go2sky    16.800s

Use sync.pool and non-concurrency safe impletement result is here

BenchmarkRandomSampler_IsSampled-10                 195834091            5.965 ns/op
BenchmarkRandomPoolSampler_IsSampled-10             190921699            6.359 ns/op