RUMM-2137 Datadog singleton #918
Conversation
There was a problem hiding this comment.
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.
dd-sdk-android/src/main/kotlin/com/datadog/android/v2/api/SDKCore.kt
Outdated
Show resolved
Hide resolved
| * SDKCore is the entry point to register Datadofg features to the core registry. | ||
| */ | ||
| @NoOpImplementation | ||
| interface SDKCore { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
dd-sdk-android/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumSessionScope.kt
Outdated
Show resolved
Hide resolved
dd-sdk-android/src/test/kotlin/com/datadog/android/rum/internal/RumFeatureTest.kt
Outdated
Show resolved
Hide resolved
dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/utils/RuntimeUtils.kt
Show resolved
Hide resolved
| * Internal implementation of the [SDKCore] interface. | ||
| * @param credentials the Datadog credentials for this instance | ||
| */ | ||
| class DatadogCore( |
There was a problem hiding this comment.
For now yes, we can discuss whether or not it should when we finalize the v2 arch
dd-sdk-android/src/test/kotlin/com/datadog/android/v2/core/DatadogCoreInitializationTest.kt
Outdated
Show resolved
Hide resolved
| @@ -38,12 +33,9 @@ import java.util.concurrent.atomic.AtomicBoolean | |||
| @Suppress("TooManyFunctions") | |||
| object Datadog { | |||
|
|
|||
| internal val initialized = AtomicBoolean(false) | |||
| internal val startupTimeNs: Long = System.nanoTime() | |||
| private var globalSDKCore: SDKCore = NoOpSDKCore() | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
xgouchet
force-pushed
the
xgouchet/RUMM-2137/datadog_singleton
branch
from
271ab4f to
15999c9
Compare
Codecov Report
@@ 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
|
What does this PR do?
Create the
DatadogCore:SDKCoreclass which (for now) contains the implementation of theDatadogsingleton. This is step 1/? of our SDKv2 migration.Next step is to make each Feature a non singleton too, which registers against the
DatadogCoreinstance.