RUM-9854: Introduce event processing thread by 0xnm · Pull Request #2631 · DataDog/dd-sdk-android

0xnm · 2025-05-06T15:57:09Z

What does this PR do?

This PR adds a dedicated context event processing thread which will be used when FeatureScope.withWriteContext is called.

Before this change when FeatureScope.withWriteContext was called, the content of the block was executed on the thread from I/O pool and block arguments were DatadogContext and EventBatchWriter.

After this change the block arguments are DatadogContext and EventWriteScope, where EventWriteScope is block-accepting lambda where argument is EventWriteScope. This allows now have 2 pipelines inside withWriteContext call:

the content of the block passed to the withWriteContext will be executed on the single context processing thread shared between all the features.
the context of the block passed to the EventWriteScope will be executed on the thread from I/O pool.

To illustrate it better:

// before
withWriteContext { datadogContext, eventBatchWriter ->
  // all on the I/O thread
  // ...
  // there is no switch to the I/O thread inside the write call, because we are already on the I/O thread
  eventBatchWriter.write(...)
}

// after
withWriteContext { datadogContext, writeScope ->
  // on the context processing thread
  // ...
  writeScope { eventBatchWriter ->
    // on the I/O thread
    eventBatchWriter.write(...)
  }
}

This will allow to introduce the wait mechanism for the feature context synchronization without imposing such wait on the client thread.

Also this PR moves withWriteContext call up to the root scope in the RUM processing, so that we can have context earlier.

The diff looks big, but its majority is in the tests where the main volume of changes comes from formatting and renaming.

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)

codecov-commenter · 2025-05-06T16:20:27Z

Codecov Report

❌ Patch coverage is 93.67470% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.13%. Comparing base (ac90182) to head (7956da7).
⚠️ Report is 218 commits behind head on nogorodnikov/rum-8170/feature-context-sync.

Files with missing lines	Patch %	Lines
...in/com/datadog/android/core/internal/SdkFeature.kt	81.25%	6 Missing ⚠️
...in/com/datadog/android/log/internal/LogsFeature.kt	85.19%	1 Missing and 3 partials ⚠️
...m/datadog/android/rum/internal/utils/SdkCoreExt.kt	90.00%	0 Missing and 2 partials ⚠️
...droid/rum/internal/domain/scope/RumSessionScope.kt	0.00%	0 Missing and 1 partial ⚠️
.../android/rum/internal/monitor/DatadogRumMonitor.kt	93.75%	0 Missing and 1 partial ⚠️
...ndroid/telemetry/internal/TelemetryEventHandler.kt	50.00%	0 Missing and 1 partial ⚠️
...y/internal/storage/SessionReplayResourcesWriter.kt	90.00%	0 Missing and 1 partial ⚠️
...dog/android/trace/internal/data/OtelTraceWriter.kt	85.71%	0 Missing and 1 partial ⚠️
...datadog/android/trace/internal/data/TraceWriter.kt	85.71%	0 Missing and 1 partial ⚠️
...id/webview/internal/log/WebViewLogEventConsumer.kt	80.00%	0 Missing and 1 partial ⚠️
... and 2 more

Additional details and impacted files

@@                              Coverage Diff                               @@
##           nogorodnikov/rum-8170/feature-context-sync    #2631      +/-   ##
==============================================================================
+ Coverage                                       70.01%   70.13%   +0.13%     
==============================================================================
  Files                                             818      819       +1     
  Lines                                           30557    30593      +36     
  Branches                                         5129     5110      -19     
==============================================================================
+ Hits                                            21392    21456      +64     
+ Misses                                           7736     7722      -14     
+ Partials                                         1429     1415      -14

Files with missing lines	Coverage Δ
...n/com/datadog/android/core/internal/CoreFeature.kt	`87.82% <100.00%> (+0.47%)`	⬆️
...n/com/datadog/android/core/internal/DatadogCore.kt	`82.35% <100.00%> (+0.08%)`	⬆️
...droid/core/internal/persistence/AbstractStorage.kt	`100.00% <100.00%> (ø)`
.../core/internal/persistence/AsyncEventWriteScope.kt	`100.00% <100.00%> (ø)`
...d/core/internal/persistence/ConsentAwareStorage.kt	`93.98% <100.00%> (-3.67%)`	⬇️
...adog/android/core/internal/utils/ConcurrencyExt.kt	`100.00% <100.00%> (ø)`
...g/android/log/internal/logger/DatadogLogHandler.kt	`77.50% <100.00%> (+0.38%)`	⬆️
...g/android/rum/internal/DatadogLateCrashReporter.kt	`84.44% <100.00%> (-0.95%)`	⬇️
...ndroid/rum/internal/domain/scope/RumActionScope.kt	`97.34% <100.00%> (-0.49%)`	⬇️
...d/rum/internal/domain/scope/RumApplicationScope.kt	`94.17% <100.00%> (-0.97%)`	⬇️
... and 17 more

... 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.

aleksandr-gringauz · 2025-05-07T08:05:53Z

+                val contextProvider = coreFeature.contextProvider
+                if (contextProvider is NoOpContextProvider) return@executeSafe
+                val context = contextProvider.context
+                val eventBatchWriteScope = storage.getEventWriteScope(context)


[minor] Looks like a duplicate code similar to the one in getWriteContextSync.

for now, it is, yes. There is a TODO item attached to the getWriteContextSync, so maybe this method will be gone by the end of the development.

aleksandr-gringauz · 2025-05-07T08:12:58Z

-                    tags = emptySet()
-                )
+        // TODO RUM-9852 Implement better passthrough mechanism for the JVM crash scenario
+        val writeContext = sdkCore.getFeature(name)


Is my understanding correct that we need this getWriteContextSync only because we have to log the crash synchronously, because otherwise the process might die before we finish (because we do it in Thread.UncaughtExceptionHandler)?

yes, this is right. That is why we have a TODO item as well, we want to have a quick passthrough for crashes avoiding switching threads. The only thread switch we can allow is for the I/O write.

mariusc83

LGTM

mariusc83 · 2025-05-07T08:11:45Z

+            // max pool size,
+            1,
+            // keep-alive time
+            0L,


why not keeping it alive for a while ?

This is to align with defaults of the Executors.newSingleThreadExecutor call (link)

aleksandr-gringauz · 2025-05-07T08:36:48Z

        }
    }

+    private fun resolveCallFullNames(resolvedCall: ResolvedCall<out CallableDescriptor>): Set<String> {


Could you please explain what this change is for? I'm just not familiar with writing custom detekt rules and don't understand what changed.

ah, I see the new tests, it is clearer now

yes, it is basically for typealias thing, because ResolvedCall will have a direct type signature, with typealias erased, so we need to generate the signature with typealias included in addition.

RUM-9854: Introduce event processing thread

7956da7

0xnm force-pushed the nogorodnikov/rum-9854/introduce-event-processing-thread branch from c5ad35e to 7956da7 Compare May 6, 2025 16:49

0xnm marked this pull request as ready for review May 6, 2025 17:29

0xnm requested review from a team as code owners May 6, 2025 17:29

aleksandr-gringauz reviewed May 7, 2025

View reviewed changes

mariusc83 approved these changes May 7, 2025

View reviewed changes

aleksandr-gringauz reviewed May 7, 2025

View reviewed changes

aleksandr-gringauz approved these changes May 7, 2025

View reviewed changes

xgouchet approved these changes May 7, 2025

View reviewed changes

0xnm merged commit fa0a135 into nogorodnikov/rum-8170/feature-context-sync May 7, 2025

0xnm deleted the nogorodnikov/rum-9854/introduce-event-processing-thread branch May 7, 2025 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RUM-9854: Introduce event processing thread#2631

RUM-9854: Introduce event processing thread#2631
0xnm merged 1 commit into
nogorodnikov/rum-8170/feature-context-syncfrom
nogorodnikov/rum-9854/introduce-event-processing-thread

0xnm commented May 6, 2025

Uh oh!

codecov-commenter commented May 6, 2025 •

edited

Loading

Uh oh!

aleksandr-gringauz May 7, 2025

Uh oh!

0xnm May 7, 2025

Uh oh!

aleksandr-gringauz May 7, 2025

Uh oh!

0xnm May 7, 2025

Uh oh!

mariusc83 left a comment

Uh oh!

mariusc83 May 7, 2025

Uh oh!

0xnm May 7, 2025

Uh oh!

aleksandr-gringauz May 7, 2025

Uh oh!

aleksandr-gringauz May 7, 2025

Uh oh!

0xnm May 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

0xnm commented May 6, 2025

What does this PR do?

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

mariusc83 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented May 6, 2025 •

edited

Loading