Add mutex to adaptiveSampler #124
Conversation
|
Changes Unknown when pulling d5adeb8 on granular_lock into ** on master**. |
|
So to restate the scenario:
|
sampler_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func updateSampler( |
There was a problem hiding this comment.
can you explain better what this is doing? It looks like it's constantly causing adaptive sampler to discard half of the endpoints.
|
yup, that's the gist of it |
sampler_test.go
Outdated
| &sampling.SamplingStrategyResponse{OperationSampling: strategiesB}, | ||
| ) | ||
| }() | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
having a test that runs 10 seconds is a very bad thing. Especially since it doesn't actually prove the lack of race condition.
There was a problem hiding this comment.
I'm not sure how else to check for a panic. Maybe I can keep the test but use t.skip() so that if you want to sanity check that everything is fine, you can run it.
There was a problem hiding this comment.
- do we need to do that check through end-to-end testing via remote sampler? Looks like we could do it only on the adaptive sampler.
- if we do the latter, what if you just run those routines a lot faster? I.e. why sleep 10us / 100us? Can we instead call Gosched() (https://golang.org/pkg/runtime/#Gosched) to yield from each goroutine, and have a small overall wait interval of say 10ms to terminate the test. If you run such test without your fix and it panics, then it's ok to keep it running permanently, 10ms isn't a big deal.
sampler_test.go
Outdated
| func isSampled(t *testing.T, remoteSampler *RemotelyControlledSampler, numOperations int, operationNamePrefix string) { | ||
| for i := 0; i < numOperations; i++ { | ||
| sampled, _ := remoteSampler.IsSampled(TraceID{}, generateRandomOperationName(numOperations, operationNamePrefix)) | ||
| assert.True(t, sampled) |
There was a problem hiding this comment.
you must yield in each iteration, otherwise there's no race, one goroutine will just run unopposed through the loop.
There was a problem hiding this comment.
interesting, when I print comments it seems like both are running simultaneously. Ill add the yield
There was a problem hiding this comment.
also, you may want to use runtime.LockOSThread(), otherwise there's still no guarantee you'd have two routines running in parallel.
Have you been able to reproduce the panic with this test?
|
Changes Unknown when pulling 69738a4 on granular_lock into ** on master**. |
|
Changes Unknown when pulling ec6dc65 on granular_lock into ** on master**. |
|
Changes Unknown when pulling ec6dc65 on granular_lock into ** on master**. |
Fixes #69
A panic might occur when multiple threads try to create a new sampler in AdaptiveSampler.IsSampled()