-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add in-memory storage support for adaptive sampling #3335
Add in-memory storage support for adaptive sampling #3335
Conversation
Signed-off-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this form the implementation has a memory leak where data will grow unbounded. Note these two things:
- The storage only needs to keep N buckets of throughput because the processor uses them with exponential decay (there is a parameter in the processor that specifies how many). So we could bound the storage by that number.
- Even though the final probabilities are appended to the storage, the processor only uses the latest record. The only reason we used to store the previous ones is to have a history of calculations for debugging purposes (it's not even exposed via API, only by looking at the database directly). So the simplest implementation would be to keep exactly one record of probabilities in memory, 2nd simplest to keep a fixed-length round-robin buffer (but then there should be a way to inspect it since there's no database we can query, e.g. by exposing via expvar).
Thanks @yurishkuro for the review. I took the default unbounded number of traces as an inspiration but given the processor implementation it makes sense to limit the number of entries to N. Regarding probabilities, I think we can go with the simplest approach first. |
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
64e8335
to
9dc6d6d
Compare
Signed-off-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
…nto srikanth/in-memory Signed-off-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #3335 +/- ##
==========================================
+ Coverage 96.01% 96.04% +0.03%
==========================================
Files 259 261 +2
Lines 15422 15464 +42
==========================================
+ Hits 14807 14852 +45
+ Misses 523 520 -3
Partials 92 92
Continue to review full report at Codecov.
|
Signed-off-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
plugin/storage/memory/sampling.go
Outdated
maxBuckets int | ||
} | ||
|
||
type Throughput struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these don't look like they need to be public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please suggest some name. I had a hard time differentiating b/w throughput
s and lowercase only made it more difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
storedThroughput?
Signed-off-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
Signed-off-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
Thanks! There's probably a place in the docs we can tweak to mention this new backend. |
It is the docs https://www.jaegertracing.io/docs/1.27/sampling/#adaptive-sampling that led me to the issue. Do you want me to send a PR updating the docs now itself or once the remaining backends are also supported? I am planning to send a follow up PR for es (and badger later). |
Hey, it's up to you. We're approaching the next release (in a week), I am not sure if there's enough time to implement other (real) backends. Maybe makes sense to just send a small PR for the |
Signed-off-by: Srikanth Chekuri srikanth.chekuri92@gmail.com
Which problem is this PR solving?