feat: Add sampling configuration for profiling #2004
Conversation
|
…/sentry-cocoa into indragiek/profiles-sample-rate
…/sentry-cocoa into indragiek/profiles-sample-rate
|
CI flakes appear to be unrelated to these changes. I'm working on a separate change to fix the flakiness of the "Generate Profiling Test Data" action, it seems to fail very regularly. |
| options.tracesSampler = { _ in return 0.49 } | ||
| options.tracesSampleRate = 0.49 | ||
| } | ||
| } | ||
|
|
||
| func testStartTransactionSamplingUsingSampleRate() { | ||
| testSampler(expected: .yes) { options in | ||
| options.tracesSampler = { _ in return 0.5 } |
There was a problem hiding this comment.
These tests are unrelated to my changes but I fixed these to set the tracesSampleRate as the name of the test suggests, instead of the sampler function.
|
All CI is passing except the profiling jobs which we know are flaking (@armcknight is working on improving those). |
* master: ci: benchmarking and profile generation improvements (#2022) release: 7.23.0 fix: Bad merge on CHANGELOG.md (#2020) fix: Log empty samples instead of collecting stacks for idle threads (#2013) fix: Handle failure to read thread priority gracefully (#2015) fix: Remove logging that could occur while a thread is suspended (#2014) fix: Use constant for max thread name size instead of magic number (#2012) Disable flaky tesDefaultMaxEnvelopesConcurrent (#2018) Fix build failure in unit tests (#2019) Fix address sanitizer compilation error (#1996) fix: Sauce labs iPhone 13 pro iOS version feat: Add transaction to baggage and trace headers (#1992) ci: fix ui thread starvation in benchmarks (#2009) fix: Add more descriptive deprecation message for enableProfiling (#2011) feat: Add sampling configuration for profiling (#2004) fix log message option name (#2006) meta: add armcknight as a codeowner (#2008) release: 7.22.0 fix: Disable broken LaunchUITests to unblock release (#2003) # Conflicts: # Samples/iOS-Swift/iOS-Swift/ViewControllers/PerformanceViewController.swift
| testProfilesSampler(expected: .yes) { options in | ||
| options.profilesSampler = { _ in return 0.51 } |
There was a problem hiding this comment.
I don't understand how some of these tests work. With a 0.51 sample rate, this should fail 49% of the time. Is something else getting mocked to make this always work? Same for a few others with a rate that's not 0.0 or 1.0.
There was a problem hiding this comment.
The tests don't use a random value. The random value is fixed see
sentry-cocoa/Tests/SentryTests/SentryHubTests.swift
Lines 779 to 781 in 45ed684
There was a problem hiding this comment.
OK, after reading SentryProfilesSampler's implementation I see how this works now, thanks.
📜 Description
Adds two new configuration options,
profilesSampleRateandprofilesSamplerto mirrortracesSampleRateandtracesSampler, except for profiling data.There are no breaking changes --
enableProfilingis now deprecated, but will continue to work for existing clients that use it (this is equivalent to settingprofilesSampleRateto1.0)💡 Motivation and Context
Currently our only control for how profiles get sampled is the transaction sampling - we always collect a profile for every sampled transaction. However, since profiles are quite a bit larger than transactions, we likely will want to undersample them relative to transactions for high data volumes. Effectively, these parameters control *for what percentage of sampled transactions do we collect a profile"
There is more info on the design of this parameter here, which we will adopt across other SDKs that support profiling as well: https://www.notion.so/sentry/Profiles-Sample-Rate-9ef72666b5af487d94cf314cc81e88c6
💚 How did you test it?
New unit tests.
📝 Checklist
🔮 Next steps