Skip to content
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

Merged
merged 35 commits into from
Jan 14, 2025
Merged

Conversation

JoshuaMoelans
Copy link
Member

@JoshuaMoelans JoshuaMoelans commented Dec 20, 2024

Fixes #864

Core changes (without tx_cxt -> tx_ctx refactor)

Copy link

github-actions bot commented Dec 20, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against e1d191e

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 97.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.67%. Comparing base (3981ea7) to head (e1d191e).
Report is 2 commits behind head on master.

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     

@JoshuaMoelans JoshuaMoelans marked this pull request as ready for review January 2, 2025 13:05
@JoshuaMoelans JoshuaMoelans marked this pull request as draft January 2, 2025 13:22
src/sentry_core.c Outdated Show resolved Hide resolved
examples/example.c Outdated Show resolved Hide resolved
examples/example.c Outdated Show resolved Hide resolved
examples/example.c Outdated Show resolved Hide resolved
@JoshuaMoelans JoshuaMoelans marked this pull request as ready for review January 7, 2025 16:25
src/sentry_core.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@supervacuus supervacuus left a 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.

examples/example.c Outdated Show resolved Hide resolved
examples/example.c Outdated Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@supervacuus supervacuus left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
examples/example.c Outdated Show resolved Hide resolved
include/sentry.h Outdated Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
examples/example.c Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
include/sentry.h Outdated Show resolved Hide resolved
include/sentry.h Outdated Show resolved Hide resolved
include/sentry.h Outdated Show resolved Hide resolved
src/sentry_core.c Outdated Show resolved Hide resolved
tests/unit/test_sampling.c Outdated Show resolved Hide resolved
examples/example.c Outdated Show resolved Hide resolved
include/sentry.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@supervacuus supervacuus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thx!

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice!

src/sentry_core.c Outdated Show resolved Hide resolved
@JoshuaMoelans JoshuaMoelans merged commit bce6e8b into master Jan 14, 2025
24 checks passed
@JoshuaMoelans JoshuaMoelans deleted the joshua/feat/tracesSampler_support branch January 14, 2025 15:42
@ShawnCZek
Copy link
Contributor

From what I understand, this part of the documentation is now obsolete:

sentry-native/include/sentry.h

Lines 1738 to 1741 in 092de88

* The second parameter is a custom Sampling Context to be used with a Traces
* Sampler to make a more informed sampling decision. The SDK does not currently
* support a custom Traces Sampler and this parameter is ignored for the time
* being but needs to be provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support tracesSampler
4 participants