Skip to content

sentry-core: add traces_sampler client option#510

Merged
kamilogorek merged 5 commits into
getsentry:masterfrom
tommilligan:traces-sampler
Nov 3, 2022
Merged

sentry-core: add traces_sampler client option#510
kamilogorek merged 5 commits into
getsentry:masterfrom
tommilligan:traces-sampler

Conversation

@tommilligan

Copy link
Copy Markdown
Contributor

This PR adds the traces_sampler option to ClientOptions, and respects it when making transaction sampling decisions as detailed in the SDK guidelines: https://develop.sentry.dev/sdk/performance/#sdk-configuration

There was some prior work on this topic in #288, however by now it's pretty stale so I started fresh.

Points for review:

  • To implement Debug for this option I just print the function's memory address (if set); I can't think of something simple that would have more value to the user. link
  • If accepted, I would like to update the official docs with the new option, could someone point me in the right direction for that?
  • Adding a new field to ClientOptions is a breaking change - I presume this necessitates a minor version bump for release?

@loewenheim

Copy link
Copy Markdown
Contributor

Thanks for doing this!

To implement Debug for this option I just print the function's memory address (if set); I can't think of something simple that would have more value to the user. link

I think that's fine for now.

If accepted, I would like to update the official docs with the new option, could someone point me in the right direction for that?

I guess you would want to add it to this page. The file to change would be this one in the docs repo.

Adding a new field to ClientOptions is a breaking change - I presume this necessitates a minor version bump for release?

According to this guideline, it even requires a major bump because you're adding a new public field, but no private field currently exists.

@tommilligan

Copy link
Copy Markdown
Contributor Author

Adding a new field to ClientOptions is a breaking change - I presume this necessitates a minor version bump for release?

According to this guideline, it even requires a major bump because you're adding a new public field, but no private field currently exists.

Right, agreed it's a breaking change - but all the sentry* packages are currently sub-v1, so I think a minor version bump is what's needed. I'll add a note in the changelog of the breakage anyway.

I don't like that adding a new config option breaks this for all users, but I think that's outside the scope of this PR.

@kamilogorek kamilogorek enabled auto-merge (squash) November 3, 2022 20:49
@codecov-commenter

codecov-commenter commented Nov 3, 2022

Copy link
Copy Markdown

Codecov Report

Merging #510 (b7d1a3e) into master (89b31f8) will increase coverage by 0.17%.
The diff coverage is 86.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
+ Coverage   78.60%   78.77%   +0.17%     
==========================================
  Files          76       76              
  Lines        9206     9240      +34     
==========================================
+ Hits         7236     7279      +43     
+ Misses       1970     1961       -9     

@kamilogorek kamilogorek merged commit 0e021ba into getsentry:master Nov 3, 2022
@tommilligan tommilligan deleted the traces-sampler branch November 4, 2022 11:33
@Turbo87

Turbo87 commented Nov 5, 2022

Copy link
Copy Markdown
Contributor

I've tried to make use of this new API, but I fail to understand how this is supposed to work. All fields on the TransactionContext struct are private, so even if the function gets passed such a struct there is still no information available to determine the sample rate for the transaction. Am I missing something? 🤔

Draft PR that shows the issue: rust-lang/crates.io#5423

@tommilligan

Copy link
Copy Markdown
Contributor Author

Hi - yes, agreed. This was actually merged by a maintainer before I'd fully tested it, so sorry for the incomplete feature.

I've opened a follow up PR already which supports inspection of the transaction context, plus adding arbitrary user data as supported in e.g. the Python SDK. Comments welcome over there! #512

This isn't a breaking change so should be able to be added with a patch release fairly quickly once merged.

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.

5 participants