feat: add sampling #940
feat: add sampling #940
Conversation
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 5a023be in 2 minutes and 3 seconds. Click for details.
- Reviewed
51lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. app-server/src/traces/consumer.rs:345
- Draft comment:
It appears thatrand::rng()might be a typo. Typically, to obtain a random number generator one might userand::thread_rng()instead. Please verify whetherrand::rng()is the intended function. - Reason this comment was not posted:
Marked as duplicate.
2. app-server/src/traces/consumer.rs:349
- Draft comment:
The methodrandom_rangedoes not seem standard. Usually, the method from therandcrate isgen_range. Consider replacingrng.random_range(0.0..1.0)withrng.gen_range(0.0..1.0)if appropriate. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_xRrrFWOK2jmQk1Fo
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| .unwrap_or(0.25_f64) | ||
| .clamp(0.0, 1.0); | ||
|
|
||
| let mut rng = rand::rngs::StdRng::from_rng(&mut rand::rng()); |
There was a problem hiding this comment.
The RNG initialization is incorrect. Replace rand::rng() with a valid RNG, e.g., use rand::thread_rng() or StdRng::from_entropy().unwrap(), to properly seed StdRng.
| let mut rng = rand::rngs::StdRng::from_rng(&mut rand::rng()); | |
| let mut rng = rand::rngs::StdRng::from_entropy(); |
There was a problem hiding this comment.
hmm there is no from_entropy method on it
| span.project_id, | ||
| e | ||
| ); | ||
| if rng.random_range(0.0..1.0) < sample_rate { |
There was a problem hiding this comment.
Use the correct RNG method. Replace rng.random_range(0.0..1.0) with rng.gen_range(0.0..1.0) or consider using rng.gen_bool(sample_rate) for clarity.
| if rng.random_range(0.0..1.0) < sample_rate { | |
| if rng.gen_range(0.0..1.0) < sample_rate { |
There was a problem hiding this comment.
random_range is the right one
Important
Add sampling to
process_batchinconsumer.rsusingTRACE_SUMMARY_SAMPLE_RATEto control trace processing.process_batchinconsumer.rsusingTRACE_SUMMARY_SAMPLE_RATEenvironment variable.rand::rngs::StdRngto generate random numbers for sampling decision.rand::{Rng, SeedableRng}inconsumer.rs.This description was created by
for 5a023be. You can customize this summary. It will automatically update as commits are pushed.