Move span.GetSamplerParams out of model/ into sampling/aggregator #6583
Conversation
Nabil-Salah
changed the title
| @@ -150,9 +151,42 @@ func (a *aggregator) HandleRootSpan(span *span_model.Span, logger *zap.Logger) { | |||
| if service == "" || span.OperationName == "" { | |||
| return | |||
| } | |||
| samplerType, samplerParam := span.GetSamplerParams(logger) | |||
| samplerType, samplerParam := GetSamplerParams(span, logger) | |||
There was a problem hiding this comment.
does not need to be public
| samplerType, samplerParam := GetSamplerParams(span, logger) | |
| samplerType, samplerParam := getSamplerParams(span, logger) |
The aggregator already has access to logger, no need to pass it, if you make the function have a receiver
There was a problem hiding this comment.
you mean by using aggregator.postAggregator.logger ?
There was a problem hiding this comment.
I mean a.getSamplerParams(span) (a method, not a function)
There was a problem hiding this comment.
yes i understand this but i mean where is the logger function to use in this method?
the only logger in struct aggregator is in a.postAggregator.logger logger
There was a problem hiding this comment.
ah, ok. Never mind then. This is a weird design that needs a cleanup externally - there's no reason for the caller of HandleRootSpan to be passing a logger.
There was a problem hiding this comment.
let's just make the function private
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6583 +/- ##
=======================================
Coverage 96.23% 96.23%
=======================================
Files 375 375
Lines 21389 21389
=======================================
Hits 20583 20583
Misses 614 614
Partials 192 192
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -335,123 +332,3 @@ func BenchmarkBatchSerialization(b *testing.B) { | |||
| } | |||
| }) | |||
| } | |||
|
|
|||
| func TestGetSamplerParams(t *testing.T) { | |||
There was a problem hiding this comment.
can you move the test to aggregator?
There was a problem hiding this comment.
by making it private it can't be tested alone but it's test will be part of HandleRootSpan test as it already uses it
There was a problem hiding this comment.
if the test is in the same package it has access to private functions
There was a problem hiding this comment.
okaay you can merge code for this #6584 issue
and I will rebase and push changes
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Nabil-Salah
force-pushed
the
span_cleaner_model
branch
from
c87d24a to
c8b5cbb
Compare
yurishkuro
changed the title
Which problem is this PR solving?
Part of [jaeger-idl] Copy model/*go to jaeger-idl/model/v1 #6571
Follow-up to Add model/v1 jaeger-idl#118
Description of the changes
How was this change tested?
go test ./model/go test ./internal/sampling/samplingstrategy/adaptive/Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test