RUM-6400 Add warning log when initializing the SDK outside of main process #2376

mariusc83 · 2024-11-05T13:21:20Z

What does this PR do?

During our escalation sessions with our clients we encountered several situations where the SDK was not initialised outside the main process. Unless the application is using 2 different processes and has 2 SDK instances (one in each process) this scenario will lead to a non - functional SDK as the data will no be uploaded as we are not initialising our schedulers in a secondary process.

In this PR we are adding a warning log for our users when the SDK is initialised in a secondary process just to better debug this situations and avoid long escalation back and forth.

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)

mariusc83 · 2024-11-05T13:22:45Z

dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/CoreFeature.kt

@@ -626,6 +626,13 @@ internal class CoreFeature(
        } else {
            appContext.packageName == currentProcess.processName
        }
+        if (!isMainProcess) {
+            internalLogger.log(


reviewers should we also send a telemetry here ? I think it is a bit overkill, what do you think ?

would it be actionable? how it would help us?

not sure it will be that actionable, that is the reason why I did not add it and just asked your opinion. I think we are better off.

I think it's an interesting data to know at least, and if we notice that there are really amount of clients doing it in background (probably meaning intentionally), the action will be making it work even if the initialization happens in background.

codecov-commenter · 2024-11-05T13:48:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.41%. Comparing base (b83e7b6) to head (97c83d1).
Report is 9 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2376      +/-   ##
===========================================
+ Coverage    70.31%   70.41%   +0.10%     
===========================================
  Files          744      744              
  Lines        27719    27724       +5     
  Branches      4631     4632       +1     
===========================================
+ Hits         19490    19520      +30     
+ Misses        6939     6926      -13     
+ Partials      1290     1278      -12

Files with missing lines	Coverage Δ
...n/com/datadog/android/core/internal/CoreFeature.kt	`87.35% <100.00%> (+0.19%)`	⬆️

... and 28 files with indirect coverage changes

0xnm · 2024-11-05T13:48:49Z

dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/CoreFeature.kt

+        internal const val SDK_INITIALIZED_IN_SECONDARY_PROCESS_WARNING_MESSAGE =
+            "Datadog SDK was initialized in a secondary process. We strongly recommend to initialize the SDK" +
+                "in the application main process otherwise the SDK would not function properly. The only scenario" +
+                "where this is acceptable is where there will be 2 instances of the SDKs running " +
+                "in the same time and one instance is guaranteed to be in the main process."


Suggested change

internal const val SDK_INITIALIZED_IN_SECONDARY_PROCESS_WARNING_MESSAGE =

"Datadog SDK was initialized in a secondary process. We strongly recommend to initialize the SDK" +

"in the application main process otherwise the SDK would not function properly. The only scenario" +

"where this is acceptable is where there will be 2 instances of the SDKs running " +

"in the same time and one instance is guaranteed to be in the main process."

internal const val SDK_INITIALIZED_IN_SECONDARY_PROCESS_WARNING_MESSAGE =

"Datadog SDK was initialized in a secondary process. We strongly recommend to initialize the SDK" +

" in the application main process otherwise the SDK would not function properly. The only scenario" +

" where this is acceptable is where there will be 2 instances of the SDKs running" +

" in the same time and one instance is guaranteed to be in the main process."

I feel though that this is very verbose. Maybe we can re-word it simpler? Like tell user that SDK must have instance initialized in the main process and that is it. And speaking of the SDK would not function properly: maybe we can just say that no data will be recorded (well, to be precise - uploaded).

This warning though raises a question why such limitation exists and why cannot we solve it (in theory we could?).

Yes I wanted to present all the possibilities in the message but I guess we can lower the amount of information. Regarding your second point there are some strong reasons why we took this approach. Let's debate about this.

xgouchet · 2024-11-05T14:55:39Z

dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/CoreFeature.kt

+            "Datadog SDK was initialized in a secondary process. We strongly recommend to initialize the SDK" +
+                "in the application main process. The sdk will not upload any data in this case."


Note
We strongly recommend to initialize the SDK in the application main process
This part of the message makes it seem like initialising the SDK in a secondary process is wrong, when it's totally legitimate to want to capture events in a non main process.
Maybe we could phrase it differently, e.g.:

Suggested change

"Datadog SDK was initialized in a secondary process. We strongly recommend to initialize the SDK" +

"in the application main process. The sdk will not upload any data in this case."

"Datadog SDK was initialized in a secondary process: although data will still be captured, nothing will be uploaded from this process." +

"Make sure to also initialize the SDK from the main process of your application."

yes I agree this makes more sense. the first message was making more sense with my initial message where I explained the difference between running it in the main or main + secondary also.

ambushwork · 2024-11-05T17:22:26Z

dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/CoreFeature.kt

+        internal const val SDK_INITIALIZED_IN_SECONDARY_PROCESS_WARNING_MESSAGE =
+            "Datadog SDK was initialized in a secondary process: although data will still be captured," +
+                "nothing will be uploaded from this process. Make sure to also initialize the SDK from the main" +
+                " process of your application."


Question: does "the main process" refers to "main thread" in Android?

In Android each process has its own JVM running code, each with several threads.

0xnm · 2024-11-06T10:08:49Z

dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/CoreFeature.kt

+            "Datadog SDK was initialized in a secondary process: although data will still be captured," +
+                "nothing will be uploaded from this process. Make sure to also initialize the SDK from the main" +


Suggested change

"Datadog SDK was initialized in a secondary process: although data will still be captured," +

"nothing will be uploaded from this process. Make sure to also initialize the SDK from the main" +

"Datadog SDK was initialized in a secondary process: although data will still be captured," +

" nothing will be uploaded from this process. Make sure to also initialize the SDK from the main" +

…ocess

mariusc83 self-assigned this Nov 5, 2024

mariusc83 commented Nov 5, 2024

View reviewed changes

mariusc83 marked this pull request as ready for review November 5, 2024 13:25

mariusc83 requested review from a team as code owners November 5, 2024 13:25

0xnm reviewed Nov 5, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-6400/add-log-for-sdk-initialization-outside-main-process branch from 1ec22e1 to fad663b Compare November 5, 2024 14:05

xgouchet approved these changes Nov 5, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-6400/add-log-for-sdk-initialization-outside-main-process branch from fad663b to 22e1d39 Compare November 5, 2024 15:09

ambushwork reviewed Nov 5, 2024

View reviewed changes

xgouchet approved these changes Nov 6, 2024

View reviewed changes

0xnm reviewed Nov 6, 2024

View reviewed changes

RUM-6400 Add warning log when initializing the sdk outside of main pr…

97c83d1

…ocess

mariusc83 force-pushed the mconstantin/rum-6400/add-log-for-sdk-initialization-outside-main-process branch from 22e1d39 to 97c83d1 Compare November 7, 2024 07:59

mariusc83 merged commit a7c4f31 into develop Nov 7, 2024
24 checks passed

mariusc83 deleted the mconstantin/rum-6400/add-log-for-sdk-initialization-outside-main-process branch November 7, 2024 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-6400 Add warning log when initializing the SDK outside of main process #2376

RUM-6400 Add warning log when initializing the SDK outside of main process #2376

mariusc83 commented Nov 5, 2024

mariusc83 Nov 5, 2024 •

edited

Loading

0xnm Nov 5, 2024

mariusc83 Nov 5, 2024

ambushwork Nov 5, 2024

codecov-commenter commented Nov 5, 2024 •

edited

Loading

0xnm Nov 5, 2024

mariusc83 Nov 5, 2024

xgouchet Nov 5, 2024

mariusc83 Nov 5, 2024

ambushwork Nov 5, 2024

xgouchet Nov 6, 2024

0xnm Nov 6, 2024

		"Datadog SDK was initialized in a secondary process. We strongly recommend to initialize the SDK" +
		"in the application main process. The sdk will not upload any data in this case."

		"Datadog SDK was initialized in a secondary process: although data will still be captured," +
		"nothing will be uploaded from this process. Make sure to also initialize the SDK from the main" +

RUM-6400 Add warning log when initializing the SDK outside of main process #2376

RUM-6400 Add warning log when initializing the SDK outside of main process #2376

Conversation

mariusc83 commented Nov 5, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

mariusc83 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusc83 Nov 5, 2024 •

edited

Loading

codecov-commenter commented Nov 5, 2024 •

edited

Loading