Refactor Adaptive Sampling Aggregator & Strategy Store #5441
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5441 +/- ##
==========================================
+ Coverage 95.50% 95.53% +0.02%
==========================================
Files 331 331
Lines 16139 16155 +16
==========================================
+ Hits 15414 15434 +20
+ Misses 552 547 -5
- Partials 173 174 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I find it difficult to reason about this change. I am on board with not moving large portions of the code across files, but I would prefer to split the functionality of the Processor more cleanly into two separate structs. We can just change the receivers in the methods to different structs to avoid large code changes:
- the parts that will be run in OTEL Processor (along with aggregator) can be isolated to a type called
PostAggregator(for now). - the parts that will be run in the OTEL Extension can be in a struct called
StrategyStore
Pushkarm029
force-pushed
the
sampling_refactoring
branch
from
4a4039d
to
5ddcaa9
Compare
|
Not able to fix these goroutine leaks: |
Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>
| // Start initializes and starts the sampling processor which regularly calculates sampling probabilities. | ||
| func (p *Processor) Start() error { | ||
| // Start initializes and starts the PostAggregator which regularly calculates sampling probabilities. | ||
| func (p *PostAggregator) Start() error { | ||
| p.logger.Info("starting adaptive sampling processor") | ||
| if err := p.electionParticipant.Start(); err != nil { | ||
| return err | ||
| } | ||
| p.shutdown = make(chan struct{}) |
There was a problem hiding this comment.
looks like there's nothing to start here, which makes sense since we just want the aggregator to call post-aggregator on every tick
| return nil | ||
| } | ||
|
|
||
| func (p *Processor) runBackground(f func()) { | ||
| func (p *PostAggregator) runBackground(f func()) { |
There was a problem hiding this comment.
Yes, but only in one function: runCalculation. I removed it and added it as inline.
|
🎉Exams Over, let's finish this. |
| go func() { | ||
| p.saveProbabilitiesAndQPS() | ||
| p.bgFinished.Done() | ||
| }() |
There was a problem hiding this comment.
why do we need to do this in the background when the parent function is already invoked in a loop?
There was a problem hiding this comment.
Running in the background allows the loop in runAggregationLoop to continue iterating without being blocked by the saving operation.
There was a problem hiding this comment.
but it is one logical operation, if you break them apart you introduce race conditions
There was a problem hiding this comment.
yes, removed it now.
Signed-off-by: pushkarm029 <pushkarmishra029@gmail.com>
## Which problem is this PR solving? - #5441 (comment) ## Description of the changes - Rename `Processor` to `PostAggregator` ## How was this change tested? - `make test` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: pushkarm029 <pushkarmishra029@gmail.com>
Which problem is this PR solving?
remotesamplingextension #5389Description of the changes
generateStrategyResponses,runCalculationLoop.loadProbabilities,runUpdateProbabilitiesLoopHow was this change tested?
make testChecklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test