-
-
Notifications
You must be signed in to change notification settings - Fork 174
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: add traces_sampler
#1108
feat: add traces_sampler
#1108
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1108 +/- ##
==========================================
+ Coverage 82.62% 82.67% +0.04%
==========================================
Files 53 53
Lines 7896 7930 +34
Branches 1234 1240 +6
==========================================
+ Hits 6524 6556 +32
- Misses 1261 1264 +3
+ Partials 111 110 -1 |
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.
Great work up to now, but a few places still need to be fixed/clarified before we can merge this. A few more tests would also make sense, especially when considering changing the order of the sampling mechanisms.
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.
Thanks for the test effort ❤️. There are only minor things left.
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.
👍 Thx!
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.
LGTM, nice!
From what I understand, this part of the documentation is now obsolete: sentry-native/include/sentry.h Lines 1738 to 1741 in 092de88
|
Fixes #864
sentry_traces_sampler_function
typedef (like theon_crash
andbefore_send
onessamplingContext
transaction_start
is done). (docs PR: feat(native): add traces_sampler support sentry-docs#12333)Core changes (without
tx_cxt
->tx_ctx
refactor)