RUM-15191: Implement ContinuousProfilingScheduler by ambushwork · Pull Request #3298 · DataDog/dd-sdk-android

ambushwork · 2026-03-26T09:58:19Z

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

ambushwork · 2026-03-26T13:04:16Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61576fe3df

ℹ️ 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".

codecov-commenter · 2026-03-26T13:25:35Z

Codecov Report

❌ Patch coverage is 68.42105% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.58%. Comparing base (f5e2767) to head (d1b1671).

Files with missing lines	Patch %	Lines
...datadog/android/profiling/internal/NoOpProfiler.kt	13.64%	19 Missing ⚠️
...dog/android/profiling/internal/ProfilingFeature.kt	50.00%	4 Missing and 8 partials ⚠️
...profiling/internal/ContinuousProfilingScheduler.kt	94.55%	2 Missing and 1 partial ⚠️
...id/profiling/internal/perfetto/PerfettoProfiler.kt	81.82%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##           feature/continuous-profiling    #3298      +/-   ##
================================================================
+ Coverage                         71.47%   71.58%   +0.11%     
================================================================
  Files                               944      947       +3     
  Lines                             34873    34967      +94     
  Branches                           5912     5924      +12     
================================================================
+ Hits                              24924    25030     +106     
+ Misses                             8285     8278       -7     
+ Partials                           1664     1659       -5

Files with missing lines	Coverage Δ
...in/java/com/datadog/android/profiling/Profiling.kt	`81.82% <100.00%> (ø)`
...com/datadog/android/profiling/internal/Profiler.kt	`100.00% <100.00%> (ø)`
...id/profiling/internal/perfetto/PerfettoProfiler.kt	`91.18% <81.82%> (-0.61%)`	⬇️
...profiling/internal/ContinuousProfilingScheduler.kt	`94.55% <94.55%> (ø)`
...dog/android/profiling/internal/ProfilingFeature.kt	`72.86% <50.00%> (+6.79%)`	⬆️
...datadog/android/profiling/internal/NoOpProfiler.kt	`13.64% <13.64%> (ø)`

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 18512a4d77

ℹ️ 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".

0xnm · 2026-03-26T16:33:11Z

+    private var pendingFuture: ScheduledFuture<*>? = null
+
+    fun start(launchProfilingActive: Boolean = false) {
+        continuousEnabled = RateBasedSampler<Unit>(sampleRate).sample(Unit)


Let's add TODO item here to make this depend on the RUM session? We anyway need to reset scheduling decision once RUM session is renewed.

the RUM session sampling applies here.

So start function only launches the continuous profiling scheduler to loop the profiling/cooldown tasks, this depends on the continuous profiling sample rate. RUM session sampling rate is checked every time the active window is about to start.

And this is where I'm not entirely getting it, because it means that we keep scheduling even if RUM session wasn't sampled

dd-sdk-android/features/dd-sdk-android-profiling/src/main/java/com/datadog/android/profiling/internal/ContinuousProfilingScheduler.kt

Lines 92 to 104 in 18512a4

// If the active window is skipped due to RUM session,

// next cooldown window still needs to be scheduled.

pendingFuture = schedulerExecutor.scheduleSafe(

operationName = OPERATION_ACTIVE_WINDOW_END,

delay = activeMs,

unit = TimeUnit.MILLISECONDS,

internalLogger = internalLogger,

runnable = {

val cooldownMs = jitter(CONTINUOUS_COOLDOWN_DURATION_MS)

logToUser { LOG_ACTIVE_WINDOW_ENDED.format(Locale.US, cooldownMs) }

scheduleCooldown(cooldownMs)

}

)

I assumed that if RUM session is not sampled, we shouldn't even continue scheduling cycles/cooldowns. Is it useful to schedule them for nothing?

Let's defer the sampling rate change once decision is made.

0xnm · 2026-03-26T16:43:04Z

+
+    fun stop() {
+        logToUser { LOG_STOPPED }
+        pendingFuture?.cancel(false)


We are using non-interruptive cancel, meaning we let already running task to complete. If I'm not wrong it is not different from shutdown call below, so maybe we can use only shutdown? This will remove the need to keep the reference to the pendingFuture.

here I keep it, since we are reusing the ScheduledExecutorService from PerfettoProfiler we should not shutdown() here, it is shut down when Profiling feature stops.

0xnm · 2026-03-26T16:45:17Z

+                appContext = appContext,
+                startReason = ProfilingStartReason.CONTINUOUS,
+                additionalAttributes = emptyMap(),
+                sdkInstanceNames = setOf(sdkCore.name),


This is where we need to refactor, because instance names are needed only for the application launch profiling to perform the broadcast, but when SDK is up and running we have an access to the complete SdkCore objects. I'm not even sure we need the support of multiple SDK cores for the normal profiler runs (but we can decide on this later).

actually providing the sdkInstanceNames here will achieve the goal that we expect, in PerfettoProfiler if it is running with a sdkInstanceName, others are rejected until it finishes.

actually providing the sdkInstanceNames here will achieve the goal that we expect

yes, but my point is about something different: we rely on the SDK names rather than on SDK instances just because SDK instances are not there yet when we start profiler. It means that if we would design Profiler API for the post-SDK initialization usage, the API could be different.

Also we introduced this for the application launch use-case where we can allow multicast to the multiple SDK cores, but this doesn't really work for the continuous profiler case where profiler run must be tied to the RUM session, but since we can afford only single profiler session we can link it to only single SDK core, and we need to adapt the code to this somehow.

And now we are mixing Profiler API (and implementation) for 2 quite different cases, making the code a bit harder to follow.

Create RUM-15324 to address that

0xnm · 2026-03-26T16:52:10Z

+        // If the active window is skipped due to RUM session,
+        // next cooldown window still needs to be scheduled.


I'm not sure I get it. If RUM session wasn't sampled, shouldn't we just stop all profiling activity?

I don't think so, if this Rum session is not sampled, we should keep scheduler idling (skip the active window) until a new session is sampled in.

If I get it right if RUM session is not sampled, this code will keep scheduling a loop scheduleCooldown -> scheduleNextCycle for nothing, doing no effective work rather than next cycle delay computation. Why not stop scheduling at all?

kikoveiga · 2026-03-26T17:04:54Z

        }
        dataWriter = createDataWriter(sdkCore)
+
+        val scheduler = ContinuousProfilingScheduler(


Should we intialize the ContinuousProfilingScheduler with a rumSessionSampled boolean? If profiling starts after RUM, is it possible to miss the initial RumSessionRenewedEvent?

it can be the case, if it happens we will need to read proactively the rum context to know if the session is tracked, I will add a todo to address it later.

0xnm · 2026-03-27T09:13:44Z

+
        // When
-        testedFeature.onReceive(fakeTTIDEvent)
+        testedFeature.onReceive(ProfilerStopEvent.TTID())


seems like we can still rely on forged fakeTTIDEvent rather than on deterministic one, it should affect the test?

0xnm · 2026-03-27T09:21:01Z

+    private var pendingFuture: ScheduledFuture<*>? = null
+
+    fun start(launchProfilingActive: Boolean = false) {
+        continuousEnabled = RateBasedSampler<Unit>(sampleRate).sample(Unit)


And this is where I'm not entirely getting it, because it means that we keep scheduling even if RUM session wasn't sampled

dd-sdk-android/features/dd-sdk-android-profiling/src/main/java/com/datadog/android/profiling/internal/ContinuousProfilingScheduler.kt

Lines 92 to 104 in 18512a4

// If the active window is skipped due to RUM session,

// next cooldown window still needs to be scheduled.

pendingFuture = schedulerExecutor.scheduleSafe(

operationName = OPERATION_ACTIVE_WINDOW_END,

delay = activeMs,

unit = TimeUnit.MILLISECONDS,

internalLogger = internalLogger,

runnable = {

val cooldownMs = jitter(CONTINUOUS_COOLDOWN_DURATION_MS)

logToUser { LOG_ACTIVE_WINDOW_ENDED.format(Locale.US, cooldownMs) }

scheduleCooldown(cooldownMs)

}

)

I assumed that if RUM session is not sampled, we shouldn't even continue scheduling cycles/cooldowns. Is it useful to schedule them for nothing?

0xnm · 2026-03-27T09:25:50Z

+                appContext = appContext,
+                startReason = ProfilingStartReason.CONTINUOUS,
+                additionalAttributes = emptyMap(),
+                sdkInstanceNames = setOf(sdkCore.name),


actually providing the sdkInstanceNames here will achieve the goal that we expect

yes, but my point is about something different: we rely on the SDK names rather than on SDK instances just because SDK instances are not there yet when we start profiler. It means that if we would design Profiler API for the post-SDK initialization usage, the API could be different.

Also we introduced this for the application launch use-case where we can allow multicast to the multiple SDK cores, but this doesn't really work for the continuous profiler case where profiler run must be tied to the RUM session, but since we can afford only single profiler session we can link it to only single SDK core, and we need to adapt the code to this somehow.

And now we are mixing Profiler API (and implementation) for 2 quite different cases, making the code a bit harder to follow.

0xnm · 2026-03-27T09:27:44Z

+        // If the active window is skipped due to RUM session,
+        // next cooldown window still needs to be scheduled.


If I get it right if RUM session is not sampled, this code will keep scheduling a loop scheduleCooldown -> scheduleNextCycle for nothing, doing no effective work rather than next cycle delay computation. Why not stop scheduling at all?

0xnm · 2026-03-30T07:48:57Z

        whenever(mockEditor.putStringSet(any(), any())) doReturn mockEditor
        whenever(mockEditor.putFloat(any(), any())) doReturn mockEditor
        testedFeature = ProfilingFeature(mockSdkCore, fakeConfiguration, mockProfiler)
+        fakeTTID = ProfilerStopEvent.TTID(fakeTTIDRumContext)


Suggested change

fakeTTID = ProfilerStopEvent.TTID(fakeTTIDRumContext)

0xnm · 2026-03-30T12:50:49Z

+        verify(mockProfiler, never()).start(any(), any(), any(), any(), any())
+        verify(mockSchedulerExecutor, never()).schedule(any<Runnable>(), any(), any())


I'm not sure, but probably the safer assert here would be verifyNoInteractions, because this test may pass if some other similar methods are actually called with slightly different method signature.

Same for similar lines in the tests below.

only mockSchedulerExecutor is updated here, the other assertions on mockProfiler are kept since there are some other interactions on it.

ambushwork force-pushed the yl/profiling/RUM-15191-continuous-profiling-scheduler branch 3 times, most recently from bf658e7 to 4a7f1ec Compare March 26, 2026 11:55

This comment has been minimized.

Sign in to view

ambushwork force-pushed the yl/profiling/RUM-15191-continuous-profiling-scheduler branch from 4a7f1ec to 61576fe Compare March 26, 2026 12:50

chatgpt-codex-connector Bot reviewed Mar 26, 2026

View reviewed changes

Comment thread ...k-android-profiling/src/main/java/com/datadog/android/profiling/internal/ProfilingFeature.kt Outdated

Comment thread ...k-android-profiling/src/main/java/com/datadog/android/profiling/internal/ProfilingFeature.kt Outdated

ambushwork force-pushed the yl/profiling/RUM-15191-continuous-profiling-scheduler branch from 61576fe to 18512a4 Compare March 26, 2026 14:42

ambushwork marked this pull request as ready for review March 26, 2026 14:55

ambushwork requested review from a team as code owners March 26, 2026 14:55

chatgpt-codex-connector Bot reviewed Mar 26, 2026

View reviewed changes

Comment thread ...ofiling/src/main/java/com/datadog/android/profiling/internal/ContinuousProfilingScheduler.kt

Comment thread ...k-android-profiling/src/main/java/com/datadog/android/profiling/internal/ProfilingFeature.kt

0xnm reviewed Mar 26, 2026

View reviewed changes

kikoveiga reviewed Mar 26, 2026

View reviewed changes

0xnm reviewed Mar 27, 2026

View reviewed changes

ambushwork force-pushed the yl/profiling/RUM-15191-continuous-profiling-scheduler branch 3 times, most recently from e0b1d62 to 3d8dfda Compare March 27, 2026 13:39

ambushwork requested review from 0xnm and kikoveiga March 27, 2026 14:16

kikoveiga previously approved these changes Mar 27, 2026

View reviewed changes

0xnm requested changes Mar 30, 2026

View reviewed changes

ambushwork dismissed kikoveiga’s stale review via d9f0980 March 30, 2026 11:36

ambushwork force-pushed the yl/profiling/RUM-15191-continuous-profiling-scheduler branch from 3d8dfda to d9f0980 Compare March 30, 2026 11:36

ambushwork requested a review from 0xnm March 30, 2026 11:43

0xnm previously approved these changes Mar 30, 2026

View reviewed changes

RUM-15191: Implement ContinuousProfilingScheduler

d1b1671

ambushwork dismissed 0xnm’s stale review via d1b1671 March 30, 2026 13:05

ambushwork force-pushed the yl/profiling/RUM-15191-continuous-profiling-scheduler branch from d9f0980 to d1b1671 Compare March 30, 2026 13:05

ambushwork requested review from 0xnm and kikoveiga March 30, 2026 13:46

0xnm approved these changes Mar 30, 2026

View reviewed changes

kikoveiga approved these changes Mar 30, 2026

View reviewed changes

ambushwork merged commit ce821f2 into feature/continuous-profiling Mar 30, 2026
26 checks passed

ambushwork deleted the yl/profiling/RUM-15191-continuous-profiling-scheduler branch March 30, 2026 14:15

	// If the active window is skipped due to RUM session,
	// next cooldown window still needs to be scheduled.
	pendingFuture = schedulerExecutor.scheduleSafe(
	operationName = OPERATION_ACTIVE_WINDOW_END,
	delay = activeMs,
	unit = TimeUnit.MILLISECONDS,
	internalLogger = internalLogger,
	runnable = {
	val cooldownMs = jitter(CONTINUOUS_COOLDOWN_DURATION_MS)
	logToUser { LOG_ACTIVE_WINDOW_ENDED.format(Locale.US, cooldownMs) }
	scheduleCooldown(cooldownMs)
	}
	)

		verify(mockProfiler, never()).start(any(), any(), any(), any(), any())
		verify(mockSchedulerExecutor, never()).schedule(any<Runnable>(), any(), any())

Uh oh!

Conversation

ambushwork commented Mar 26, 2026

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

This comment has been minimized.

ambushwork commented Mar 26, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Mar 26, 2026 •

edited

Loading