-
-
Notifications
You must be signed in to change notification settings - Fork 68
feat(sentry): Reduce sample rate for specific endpoints #399
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
feat(sentry): Reduce sample rate for specific endpoints #399
Conversation
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| traces_sampler: Some(Arc::new(|ctx: &sentry::TransactionContext| { | ||
| let transaction_name = ctx.name(); |
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.
Provide SamplingContext and f32 in traces sampler
ClientOptions::traces_sampler expects a callback of type Fn(&SamplingContext) -> f32, but the added closure is declared as |ctx: &sentry::TransactionContext| and returns 0.001/0.01 literals that default to f64. The mismatch in both the argument and return types prevents the crate from compiling. The sampler should accept &SamplingContext and return f32 values.
Useful? React with 👍 / 👎.
Pull Request Test Coverage Report for Build 18507385068Details
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 18507385068Details
💛 - Coveralls |
|
@iambriccardo nice that you were looking at this. Once thing that was weird was that the ELB health checker was not hitting the
Maybe the health checker is not configured correctly. |
|
Ehm, this is likely a problem of the transaction parsing. Will take a look next week! |
I did a more in depth analysis and it seems to be working fine (also no more of these transactions are coming in). I would merge this PR and then monitor the situation. |
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.
Okay, let's merge it.


This PR reduces the sample rate for specific endpoints that are pinged often in ETL.