feat: Add transaction sampling properties to SentryOptions

[brustolin]

📜 Description

Added tracesSampleRate and tracesSampler to SentryOptions

💡 Motivation and Context

Options required to determine sampling of transactions.

💚 How did you test it?

No new behavior added.

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the CHANGELOG if needed
• I updated the docs if needed
• Review from the native team if needed
• No breaking changes

🔮 Next steps

Apply transaction sampling rules on SentryHub startTransaction.

[philipphofmann]

I think there are still some discrepancies to the other SDKs in the API, but maybe these are wanted. Anyway, I added comments for all of them. If I'm wrong, just let me know. The code in general looks good. Tests are still missing, but I guess you are going to add them soon 😀

[philipphofmann]

m: Can we mention that the default value is 1.0. I actually prefer making this nonnull, as the default can be 1.0.

[bruno-garcia]

The default shouldn't be 1.0 though. It's 0.

Default is null so we can know if the user has defined a value explicilty. This discussion happened already in other SDKs and this value is nullable in Java:

https://github.com/getsentry/sentry-java/blob/4620134a84de5e8a10a2811392f31941a10059cc/sentry/src/main/java/io/sentry/SentryOptions.java#L146

Seems this is different in .NET which is non-nullable:
https://github.com/getsentry/sentry-dotnet/blob/06712edbe7100eb900676ea5a9b4473584d1c717/src/Sentry/SentryOptions.cs#L396-L403

Again this idea of setting such setting nullable is that it allow knowing if the user has configured it via the init callback explicitly, or if not, we can bind the value from a configuration file or some integration.

[brustolin]

@bruno-garcia, objc doesn't have nullable primitives. Another discussion is required to solve this.

[bruno-garcia]

Take the .NET approach then, we had discussions there.

IIRC: We use the tracerSamplingCallback first if it exists, if that returns null (it must be able to return nullable there btw!) or if it doesn't exist (the callback is null on the options) it falls back to the random sampling.
/cc @marandaneto @Tyrrrz

[brustolin]

Well, I can try to use the callback first if it's not null, but the same problem applies here, I can't have a callback that returns nullable double, so if the user decides to use a callback it will have to decide the sampling in the callback.

We have an alternative option to use NSNumber instead of double, which is nullable. But then in Swift will be NSNumber as well, which in some cases doesn't have an automatically bridge to/from double.
I don't like the idea of using this objc classes in Swift, some new developers are not used to these classes.

[philipphofmann]

I like the option @marandaneto proposed unless you @brustolin think this is weird for Swift users.

[brustolin]

After a lot of consideration, I realized that sampleRate is already NSNumber, so the SDK user already must deal with this kind of field.
Let's throw another one and make use of the null capability.

[marandaneto]

wfm

[marandaneto]

unresolving so I link copy-paste the comment link, but ignore my comment

[armcknight]

so we can know if the user has defined a value explicilty

@bruno-garcia if they are still accessible, can you link to the discussions you mentioned that cover this point? I couldn't find any discussion of this in the Java PR you linked. I see no reason the SDK should behave differently whether a customer explicitly sets the sample rate to 0, vs never having set it at all, which would leave it at the default, which is also 0. At least, I see nowhere in the SDK that cares about the difference, or that we ever report whether or not it's explicitly set to the backend.

The fact that -[nil doubleValue] resolves to 0 is an implementation detail we should not rely on. It also obscures the intention of the design in our code. nil should be reserved for something that's unknowable at an API interface, like if we need to retrieve it from a network call or file, but those tasks fail, in which case we'd have to pick a default fallback value anyways, which would be 0 here.

[bruno-garcia]

Next up we need to use these sample rates.

Some ideas for tests:
https://github.com/getsentry/sentry-dotnet/blob/cee8cc337cec426373c44521c4421e93d4b5c108/test/Sentry.Tests/HubTests.cs#L189-L432

[bruno-garcia]

LGTM

[philipphofmann]

Just two things to resolve. Apart from that, we are good.

[marandaneto]

other than the open comments, LGTM

[bruno-garcia]

🥳 🥳 🥳 🥳 🥳