feat(launch profiling): use dedicated traces instead of auto perf traces #3621
Conversation
…age up the profile
this is primarily to fix the integration test, but if there is any
logic in the profiler that relies on the value of that variable,
it needs to be correct
since the only thing we need that is swizzled is the automatic UIViewController tracking
…launch-profiling2
…/launch-profiling3
…/launch-profiling2
…re its used for the uivc tracker installation
…launch-profiling2
…/launch-profiling3
…/launch-profiling2
…launch-profiling2
…launch-profiling2
…/launch-profiling3
…/launch-profiling2
…/launch-profiling3-create-dedicated-launch-traces
There was a problem hiding this comment.
I think we should keep the old behavior of trying to attach the app launch profile to the app start transaction. Having both linked in Sentry is highly useful for our users, or do you think it's acceptable to have an empty transaction with the app launch profile? Am I missing something? I would only create a dedicated transaction for the edge case you described here:
If launch A evaluates a tracesSampleRate of 1, and stores that in the launch config file, then launch B runs the launch profile but tracesSampleRate evaluates to 0, then SentryUIViewControllerPerformanceTracker isn't installed and never tries to start the ui.load trace.
Maybe it's also acceptable to just ignore this for now and not send a profile if the traces sample rate is 0. We could point that out in the docs. In the long run, we want to remove the dependency of profiles and transactions, if I'm not mistaken. I hope that continuous profiling doesn't rely on transactions anymore #3555.
Side note:
It's a bit strange that we change the logic in SentryLaunchProfiling, SentryHub, SentrySDK, SentryTracer, but the code doesn't require any changes in the tests. I would expect tests to fail when we change the behavior, or do you plan on adding them later?
What I'd like to avoid is the situation where we're already running the profiler, incurring performance overhead during the launch, and then throwing out the profile because the new traces sample rate evaluates to 0; this also means there's no trace to stop, which would stop the profiler, which instead runs until it times out after 30 seconds of wasted work.
There was one integration test still failing, which started working after i made this change, which sets up the scenario I describe. I also removed some other tests that deal with the "backup config file" that is no longer a thing.
You are correct, they will become completely decoupled. |
…launch-profiling3-create-dedicated-launch-traces
I fully understand that. What about my suggestion to attach the profile to the app start transaction per default and only start your fallback app launch profile transaction when the trace sample rate evaluates to 0 on a subsequent launch? Then, we attach the app launch profile most of the time to the app start transaction so users can see them linked in Sentry. Attaching the profile always to an extra transaction is not a good idea because of an edge case. On the other hand, it could be quite confusing to see the profile once here and there. I leave this decision up to you. I'm okay with either way, as the strategy will most likely change when we have continuous profiling. This discussion shouldn't stop us from shipping this feature.
Then, all good. The open merge conflicts make this PR hard to review. I remember it being small, but we have now plenty of changes in here Overall, the PR looked good to me, but I would like to have another proper look without the conflicts before approving it. |
|
It looks good, but need to fix CI. |
…3633) Co-authored-by: Philipp Hofmann <[email protected]>
|
|
||
| There's only ever one profiler instance running at a time, but instances that have timed out will be kept in memory until all traces that ran concurrently with it have finished and serialized to envelopes. The associations between profiler instances and traces are maintained in `SentryProfiledTracerConcurrency`. | ||
|
|
||
| App launches can be automatically profiled if configured with `SentryOptions.enableAppLaunchProfiling`. If enabled, when `SentrySDK.startWithOptions` is called, `SentryLaunchProfiling.configureLaunchProfiling` will get a sample rate for traces and profiles with their respective options, and store those rates in a file to be read on the next launch. On each launch, `SentryLaunchProfiling.startLaunchProfile` checks for the presence of that file is used to decide whether to start an app launch profiled trace, and afterwards retrieves those rates to initialize a `SentryTransactionContext` and `SentryTracerConfiguration`, and provides them to a new `SentryTracer` instance, which is what actually starts the profiler. There is no hub at this time; also in the call to `SentrySDK.startWithOptions`, any current profiled launch trace is attempted to be finished, and the hub that exists by that time is provided to the `SentryTracer` instance via `SentryLaunchProfiling.stopLaunchProfile` so that when it needs to transmit the transaction envelope, the infrastructure is in place to do so. |
There was a problem hiding this comment.
Thanks for the readme entry 💯
armcknight
merged commit 856083f
into
armcknight/feat/launch-profiling
armcknight
deleted the
armcknight/feat/launch-profiling3-create-dedicated-launch-traces
branch
Follows #3606 and followed by #3633
Realized there are some unworkable edge cases with trying to use
ui.loadtraces to attach launch profiles as generated from SentryUIViewControllerPerformanceTracker. If launch A evaluates a tracesSampleRate of 1, and stores that in the launch config file, then launch B runs the launch profile but tracesSampleRate evaluates to 0, then SentryUIViewControllerPerformanceTracker isn't installed and never tries to start the ui.load trace.Instead, just create a dedicated transaction to attach the profile to. This greatly simplifies several places where we previously tried to thread the sampling decisions through to make sure the auto tx would not be sampled out. This also allows SwiftUI apps to get launch traces (as originally discussed in the TODO section of #3529 and the comment at #3529 (comment))
#skip-changelog