RUM-15190: Extend app launch profiling duration for continuous merge#3277
RUM-15190: Extend app launch profiling duration for continuous merge#3277ambushwork wants to merge 1 commit intofeature/continuous-profilingfrom
Conversation
5726ae9 to
852ee36
Compare
852ee36 to
0c9ffdc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c9ffdc8fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runnable = { | ||
| stopSignal?.let { signal -> | ||
| if (profilingStartReason == ProfilingStartReason.APPLICATION_LAUNCH && | ||
| !signal.isCanceled && continuousSampler.sample(Unit) |
There was a problem hiding this comment.
Cancel app-launch profiling only when continuous sampling is off
The timer condition is inverted: signal.cancel() runs when continuousSampler.sample(Unit) is true, which means sessions sampled into continuous profiling are stopped at 10s, while non-sampled sessions keep running up to the 60s request duration. This is the opposite of the stated behavior (“stop at 10s if continuous profiling is not sampled”) and will skew runtime/overhead and break the intended app-launch→continuous merge behavior, especially with default non-100% continuous sample rates.
Useful? React with 👍 / 👎.
What does this PR do?
This PR changes the max duration of app launch profiling request from 10s -> (ttid, 1 min] in order to be able to extend as the continuous profiling, at the same time a timer of 10s is introduced to prevent the application launch profiling to go beyond 10 seconds if continuous profiling is not sampled in.
Motivation
RUM-15190
Review checklist (to be filled by reviewers)