Add tracer sampler #73
Conversation
sampler.go
Outdated
| type RandomSampler struct { | ||
| samplingRate float64 | ||
| rand *rand.Rand | ||
| Threshold int |
There was a problem hiding this comment.
This is just for test, random sampler is not easy to test the correctness
There was a problem hiding this comment.
changed Threshold to threshold, sorry I am newer to golang
sampler.go
Outdated
|
|
||
| func NewRandomSampler(samplingRate float64) *RandomSampler { | ||
| if samplingRate < 0.0 || samplingRate > 1.0 { | ||
| return nil |
There was a problem hiding this comment.
I think this is not a good practice, the user would expect nil when use sampling unless you have an err status.
There was a problem hiding this comment.
How about <=0 and >=1.0 use NewConstSampler?
There was a problem hiding this comment.
Ok, move the logic to WithSampler
segment.go
Outdated
| @@ -237,7 +237,11 @@ func newSegmentRoot(segmentSpan *segmentSpanImpl) *rootSegmentSpan { | |||
| break | |||
| } | |||
| } | |||
| s.tracer.reporter.Send(append(s.segment, s)) | |||
| //check need send segment with sample service | |||
| sampled := s.tracer.sampler.IsSampled(s.TraceID, s.FirstSpan.GetOperationName()) | |||
There was a problem hiding this comment.
- Should be sampled when the first
Spanis created, andNoopSpanis returned when ignored. - Sampling should be forced when downstream services are sampled.
Hi @xbkaishui, that's my suggestion, what do you think?
There was a problem hiding this comment.
Hi @arugal, thanks for your cr, I have a couple of questions about what your mention
for the first one, how to decide which one is the first span ?
for the second one, how to check when downstream services are sampled, if it is have sw8, I should consider it is force sampled?
There was a problem hiding this comment.
how to decide which one is the first span ?
Lines 116 to 119 in fdae247
When parentSpan is nil.
how to check when downstream services are sampled, if it is have sw8, I should consider it is force sampled?
Yes, when sw8 header exist.
I think that the code should be added to trace#CreateLocalSpan or trace#createNoop.
There was a problem hiding this comment.
Hi @arugal Thanks for you help, changed as your suggestion. please help review again
There was a problem hiding this comment.
-
CreateEntrySpanandCreateExitSpanshould judge theCreateLocalSpanreturnsNoopSpanor not. SpecificallyCreateExitSpan, ifNoopSpanreturnsspan type is wrongerror. -
traceIDis not required forIsSampled, please recheck.
move sample logic to CreateEntrySpan
|
@arugal, I have changed the logic , please help to check if it is acceptable, thanks |
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
==========================================
- Coverage 69.59% 69.12% -0.47%
==========================================
Files 12 13 +1
Lines 638 677 +39
==========================================
+ Hits 444 468 +24
- Misses 158 171 +13
- Partials 36 38 +2
Continue to review full report at Codecov.
|
|
@arugal , changed as your request, please help to check again. Thanks |
|
can this commit merger into v0.3 & v0.4 branch ? |
|
what shoud i do if i want tracer sampler but the oap version must be v6.4.0? is it possible to new a branch for adapting the old grcp protocol? |
Fix #70 , support client side trace sampling service.