Migrate Android SDK to v2 #522

louiszawadzki · 2023-09-04T08:55:35Z

What does this PR do?

Migrate Android SDK to v2.
We always use the default instance of the SDK, later we'll see if we can allow multiple instances.

Additional Notes

iOS will follow in another PR.

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests
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)
If this PR is auto-generated, please make sure also to manually update the code related to the change

louiszawadzki · 2023-09-04T09:07:47Z

packages/core/android/build.gradle

@@ -204,7 +207,6 @@ ktlint {
  outputToConsole.set(true)
  ignoreFailures.set(false)
  enableExperimentalRules.set(false)
-  additionalEditorconfigFile.set(file("${project.rootDir}/script/config/.editorconfig"))


this option does not exist anymore and the file did not exist

louiszawadzki · 2023-09-04T09:17:26Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DatadogSDKWrapper.kt

@@ -53,22 +76,38 @@ internal class DatadogSDKWrapper : DatadogWrapper {
    }

    override fun telemetryDebug(message: String) {
-        Datadog._internal._telemetry.debug(message)
+        // Do not initialize the telemetry proxy before SDK is initialized


I did not find an easy way to test this.
It can be a bit dangerous, as if the telemetry (or webview) proxy is initialized before the SDK, then telemetry (or webview) proxy will never work.

I don't know what's the impact of re-creating a proxy on every telemetry and webview event, let me know if you think this logic is worth it or not.

proxy won't be recreated on every telemetry event, because lazy does initialization only once and then memoized value is returned each time.

However, maybe we can optimize isInitialized call? Datadog.isInitialized is thread-safe call, but if this safety overhead is not needed and this wrapper is called only from the single thread always, maybe we can store initialization flag directly here?

louiszawadzki · 2023-09-04T09:19:41Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DdTraceImplementation.kt

-    private val tracerProvider: () -> Tracer = { AndroidTracer.Builder().build() }
+    private val tracerProvider: () -> Tracer = {
+        val tracer = AndroidTracer.Builder().build()
+        GlobalTracer.registerIfAbsent(tracer)


Not sure if this is needed, need to be tested in real

yes, it is still needed. We cannot move it behind .enable call as it was done for RUM, because GlobalTracer is not owned by us (it is coming from io.opentracing) and may be already used by the user for something else.

0xnm · 2023-09-04T12:47:54Z

packages/core/android/src/test/kotlin/com/datadog/tools/unit/MockRumMonitor.kt

+        type: com.datadog.android.rum.RumActionType,
+        name: kotlin.String,
+        attributes: kotlin.collections.Map<kotlin.String, kotlin.Any?>


we shouldn't use FQCN directly in this file, we should use imports instead.

0xnm · 2023-09-04T12:54:44Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DatadogSDKWrapper.kt

    override fun addRumGlobalAttributes(attributes: Map<String, Any?>) {
        attributes.forEach {
-            GlobalRum.addAttribute(it.key, it.value)
+            this.getRumMonitor().addAttribute(it.key, it.value)


minor: we can get rumMonitor just once, outside of forEach

0xnm · 2023-09-04T12:56:54Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DdSdkImplementation.kt

+        val sdkConfiguration = buildSDKConfiguration(ddSdkConfiguration)
+        val rumConfiguration = buildRUMConfiguration(ddSdkConfiguration)


just for the naming consistency:

Suggested change

val sdkConfiguration = buildSDKConfiguration(ddSdkConfiguration)

val rumConfiguration = buildRUMConfiguration(ddSdkConfiguration)

val sdkConfiguration = buildSdkConfiguration(ddSdkConfiguration)

val rumConfiguration = buildRumConfiguration(ddSdkConfiguration)

0xnm · 2023-09-04T13:00:05Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DdSdkImplementation.kt

+            additionalConfig?.filterValues { it != null }?.mapValues {
+                it.value!!
+            }
+                ?: emptyMap()


minor: a bit strange formatting here

0xnm · 2023-09-04T13:01:46Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DdTraceImplementation.kt

-    private val tracerProvider: () -> Tracer = { AndroidTracer.Builder().build() }
+    private val tracerProvider: () -> Tracer = {
+        val tracer = AndroidTracer.Builder().build()
+        GlobalTracer.registerIfAbsent(tracer)


yes, it is still needed. We cannot move it behind .enable call as it was done for RUM, because GlobalTracer is not owned by us (it is coming from io.opentracing) and may be already used by the user for something else.

0xnm · 2023-09-04T13:55:03Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DatadogSDKWrapper.kt

@@ -53,22 +76,38 @@ internal class DatadogSDKWrapper : DatadogWrapper {
    }

    override fun telemetryDebug(message: String) {
-        Datadog._internal._telemetry.debug(message)
+        // Do not initialize the telemetry proxy before SDK is initialized


proxy won't be recreated on every telemetry event, because lazy does initialization only once and then memoized value is returned each time.

However, maybe we can optimize isInitialized call? Datadog.isInitialized is thread-safe call, but if this safety overhead is not needed and this wrapper is called only from the single thread always, maybe we can store initialization flag directly here?

0xnm

lgtm, I left some non-blocking suggestions

0xnm · 2023-09-06T09:57:52Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DdSdkImplementation.kt

+            additionalConfig?.filterValues { it != null }?.mapValues {
+                it.value!!
+            }
+                ?: emptyMap()


Suggested change

additionalConfig?.filterValues { it != null }?.mapValues {

it.value!!

}

?: emptyMap()

additionalConfig?.filterValues { it != null }?.mapValues {

it.value!!

} ?: emptyMap()

0xnm · 2023-09-06T09:58:13Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DdSdkImplementation.kt

+            (configuration.additionalConfig?.get(DD_FIRST_PARTY_HOSTS) as? ReadableArray)
+                ?.toArrayList() as?
+                    List<ReadableMap>


Suggested change

(configuration.additionalConfig?.get(DD_FIRST_PARTY_HOSTS) as? ReadableArray)

?.toArrayList() as?

List<ReadableMap>

(configuration.additionalConfig?.get(DD_FIRST_PARTY_HOSTS) as? ReadableArray)

?.toArrayList() as? List<ReadableMap>

0xnm · 2023-09-06T09:58:37Z

packages/core/android/src/main/kotlin/com/datadog/reactnative/DatadogSDKWrapper.kt

    override fun addRumGlobalAttributes(attributes: Map<String, Any?>) {
+        val rumMonitor = this.getRumMonitor()


Suggested change

val rumMonitor = this.getRumMonitor()

val rumMonitor = getRumMonitor()

louiszawadzki · 2023-09-20T08:57:04Z

Closing this PR to merge directly all v2 changes in #523

louiszawadzki added 9 commits August 31, 2023 14:07

Update Android SDK to v2

8a70f02

Extract features enablement to Datadog Wrapper

b5bce9b

Fix NoOpViewTrackingStrategy after v2

dde1092

Fix Kotlin upgrade errors

c104c15

Get RUM monitor from DD Wrapper

ab6bc2c

Make applicationId mandatory in android config

a5298d6

Fix tests after v2 upgrade

d25d243

Do not recreate proxies at every call

798ae72

Update ktlint

e36026f

louiszawadzki force-pushed the louiszawadzki/migrate-native-sdk-v2 branch from 290f6cf to e36026f Compare September 4, 2023 08:56

louiszawadzki changed the title ~~Migrate native sdk to v2~~ Migrate Android SDK to v2 Sep 4, 2023

louiszawadzki commented Sep 4, 2023

View reviewed changes

louiszawadzki marked this pull request as ready for review September 4, 2023 12:37

louiszawadzki requested a review from a team as a code owner September 4, 2023 12:37

0xnm reviewed Sep 4, 2023

View reviewed changes

xgouchet previously approved these changes Sep 5, 2023

View reviewed changes

louiszawadzki added 3 commits September 6, 2023 11:07

Get RUM Monitor only once when adding attributes

23a619f

Rename configuration builders

6cca754

Clean imports in MockRumMonitor

7136dd9

louiszawadzki dismissed xgouchet’s stale review via 7136dd9 September 6, 2023 09:50

0xnm approved these changes Sep 6, 2023

View reviewed changes

louiszawadzki closed this Sep 20, 2023

louiszawadzki deleted the louiszawadzki/migrate-native-sdk-v2 branch September 20, 2023 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Android SDK to v2 #522

Migrate Android SDK to v2 #522

louiszawadzki commented Sep 4, 2023 •

edited

Loading

louiszawadzki Sep 4, 2023

louiszawadzki Sep 4, 2023

0xnm Sep 4, 2023

louiszawadzki Sep 4, 2023

0xnm Sep 4, 2023

0xnm Sep 4, 2023

0xnm Sep 4, 2023

0xnm Sep 4, 2023

0xnm Sep 4, 2023

0xnm Sep 4, 2023

0xnm Sep 4, 2023

0xnm left a comment

0xnm Sep 6, 2023

0xnm Sep 6, 2023

0xnm Sep 6, 2023

louiszawadzki commented Sep 20, 2023

		val sdkConfiguration = buildSDKConfiguration(ddSdkConfiguration)
		val rumConfiguration = buildRUMConfiguration(ddSdkConfiguration)

		override fun addRumGlobalAttributes(attributes: Map<String, Any?>) {
		val rumMonitor = this.getRumMonitor()

	val rumMonitor = this.getRumMonitor()
	val rumMonitor = getRumMonitor()

Migrate Android SDK to v2 #522

Migrate Android SDK to v2 #522

Conversation

louiszawadzki commented Sep 4, 2023 • edited Loading

What does this PR do?

Additional Notes

Review checklist (to be filled by reviewers)

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

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

0xnm left a 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

louiszawadzki commented Sep 20, 2023

louiszawadzki commented Sep 4, 2023 •

edited

Loading