RUMM-2137 Datadog singleton by xgouchet · Pull Request #918 · DataDog/dd-sdk-android

xgouchet · 2022-04-29T12:35:27Z

What does this PR do?

Create the DatadogCore:SDKCore class which (for now) contains the implementation of the Datadog singleton. This is step 1/? of our SDKv2 migration.
Next step is to make each Feature a non singleton too, which registers against the DatadogCore instance.

0xnm

Nice move towards SDK v2! I left mostly some questions and few minor suggestions.

Also do we want this PR to target develop instead sdk_v2? There is indeed no change to the public API yet, but then we need to complete at least internal changes started by this PR until the release.

0xnm · 2022-04-29T14:52:53Z

+ * SDKCore is the entry point to register Datadofg features to the core registry.
+ */
+@NoOpImplementation
+interface SDKCore {


I would propose also to maybe create some Internal annotation which will raise a warning on the consumer side if interface annotated with that is used. Simply just to let know that even if things in api package are public, they are not meant for the use on the customer side, at least yet. WDYT?

The only thing that might work is using the @RequireOptIn annotation, but not sure if it'll work for people using Java. Also not sure if we want to hide those. It's just an interface, and if someone wants to track something we don't yet track they can make their own SDK feature.

Makes sense if we want to give users a possibility to create their own features. I was more thinking to have this annotation for a while until API is not stable and we are 100% sure it works well and is final.

Regarding the Java usage: I think we can safely assume that majority of the customers write new code in Kotlin, so Kotlin compiler will be involved.

0xnm · 2022-04-29T15:11:26Z

+ * Internal implementation of the [SDKCore] interface.
+ * @param credentials the Datadog credentials for this instance
+ */
+class DatadogCore(


should this class be public?

For now yes, we can discuss whether or not it should when we finalize the v2 arch

ncreated · 2022-04-29T16:20:22Z


-    internal val initialized = AtomicBoolean(false)
-    internal val startupTimeNs: Long = System.nanoTime()
+    private var globalSDKCore: SDKCore = NoOpSDKCore()


Good for now 👍. Is it that we can't yet move it to

com.datadog.android.v2.api.SDKCore

because Datadog is an object? Or it's there only now for simplicity?

Basically everything will be moved to DatadogCore and Datadog will just be a facade to keep API compatibility. What do you want to put in SDKCore?

So the way I understand, later when we will support multiple SDKs in the same process we will have more instances of the SdkCore based on the configuration right ? Each one with its own registered features.

I only mean the globalSDKCore (the variable) itself. Soon, it will be used in feature modules to obtain the feature reference (for further accessing storage in data collectors), e.g.:

// in RUM monitor: val feature = globalSDKCore.getFeature(featureName: "rum")

To avoid having com.datadog.android.rum.internal depend on com.datadog.android.Datadog for obtaining this reference, and instead make it depend solely on com.datadog.android.v2.api we need the SDKs registry (globalSDKCore) to be defined in *.v2.api.

Hmmm yes but the thing is that eventually this global registry won't exist anymore, so I prefer to keep it here and not add singleton in the new API that is there to avoid using singletons anyway

Clear enough 👍

0xnm

Some minor changes are required based on comments (typo, redundant nullability type, unused import), but overall lgtm.

codecov-commenter · 2022-05-03T08:40:41Z

Codecov Report

Merging #918 (15999c9) into feature/sdkv2 (7588259) will decrease coverage by 0.16%.
The diff coverage is 86.27%.

@@                Coverage Diff                @@
##           feature/sdkv2     #918      +/-   ##
=================================================
- Coverage          83.04%   82.88%   -0.16%     
=================================================
  Files                267      270       +3     
  Lines               9050     9071      +21     
  Branches            1454     1453       -1     
=================================================
+ Hits                7515     7518       +3     
- Misses              1139     1154      +15     
- Partials             396      399       +3

Impacted Files	Coverage Δ
...g/android/v2/api/SDKFeatureStorageConfiguration.kt	`0.00% <0.00%> (ø)`
...og/android/v2/api/SDKFeatureUploadConfiguration.kt	`0.00% <0.00%> (ø)`
...oid/src/main/kotlin/com/datadog/android/Datadog.kt	`72.73% <87.50%> (-9.73%)`	⬇️
.../kotlin/com/datadog/android/v2/core/DatadogCore.kt	`87.80% <87.80%> (ø)`
...n/com/datadog/android/core/internal/CoreFeature.kt	`93.39% <100.00%> (+0.03%)`	⬆️
...atadog/android/core/internal/utils/RuntimeUtils.kt	`100.00% <100.00%> (ø)`
...og/android/log/internal/logger/LogcatLogHandler.kt	`77.27% <100.00%> (-18.08%)`	⬇️
...lin/com/datadog/android/rum/internal/RumFeature.kt	`91.75% <100.00%> (+0.09%)`	⬆️
...droid/rum/internal/domain/scope/RumSessionScope.kt	`94.20% <100.00%> (ø)`
...android/log/internal/logger/TelemetryLogHandler.kt	`75.00% <0.00%> (-25.00%)`	⬇️
... and 14 more

xgouchet added 2 commits April 29, 2022 14:26

RUMM-2137 create DatadogCore class

04369ac

RUMM-2137 Switch to non singleton DatadogCore

68e65ed

xgouchet requested a review from a team as a code owner April 29, 2022 12:35

xgouchet added the size-large This PR is large sized label Apr 29, 2022

0xnm reviewed Apr 29, 2022

View reviewed changes

ncreated reviewed Apr 29, 2022

View reviewed changes

xgouchet changed the base branch from develop to feature/sdkv2 May 2, 2022 07:39

xgouchet requested review from 0xnm, mariusc83 and ncreated May 2, 2022 07:40

mariusc83 approved these changes May 2, 2022

View reviewed changes

0xnm requested changes May 2, 2022

View reviewed changes

0xnm approved these changes May 2, 2022

View reviewed changes

ncreated approved these changes May 2, 2022

View reviewed changes

RUMM-2137 fix code review comments

15999c9

xgouchet force-pushed the xgouchet/RUMM-2137/datadog_singleton branch from 271ab4f to 15999c9 Compare May 3, 2022 08:22

xgouchet merged commit fbcc3d1 into feature/sdkv2 May 3, 2022

xgouchet deleted the xgouchet/RUMM-2137/datadog_singleton branch May 3, 2022 08:47

xgouchet added this to the 1.16.0 milestone Dec 13, 2023

Uh oh!

Conversation

xgouchet commented Apr 29, 2022

What does this PR do?

Uh oh!

0xnm left a comment

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!

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!

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!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented May 3, 2022 •

edited

Loading