Migrate native SDKs to v2 #523

louiszawadzki · 2023-09-06T14:52:50Z

What does this PR do?

Migrate native SDKs to v2.

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

fuzzybinary

Not gonna lie, I skimmed the changes to the main unit test files

I may come back and do a second pass on those files specifically.

fuzzybinary · 2023-09-15T12:10:02Z

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

@@ -53,22 +77,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
+        if (isInitialized()) {


Could you simplify this a bit and have telemetryProxy return an optional, then move the check into that accessor? Same with webViewProxy.

fuzzybinary · 2023-09-15T12:17:44Z

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

@@ -274,7 +240,7 @@ class DdSdkImplementation(
            }
        )

-        _InternalProxy.setTelemetryConfigurationEventMapper(
+        _RumInternalProxy.setTelemetryConfigurationEventMapper(


Weird that Android kept this and iOS didn't...

fuzzybinary · 2023-09-15T12:21:34Z

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

@@ -7,13 +7,14 @@
 package com.datadog.reactnative

 import android.content.Context
+import com.datadog.android.api.SdkCore
 import com.datadog.android.rum.tracking.ViewTrackingStrategy

 /**
 * No-op implementation of the [ViewTrackingStrategy].
 */
 object NoOpViewTrackingStrategy : ViewTrackingStrategy {


Isn't NoOpViewTrackingStrategy available in the SDK now?

Unfortunately it's internal.

maybe we could make it public 🤔

fuzzybinary · 2023-09-15T12:28:52Z

packages/core/ios/Sources/DdSdkImplementation.swift

        self.mainDispatchQueue = mainDispatchQueue
        self.jsDispatchQueue = jsDispatchQueue
        self.jsRefreshRateMonitor = jsRefreshRateMonitor
+        self.RUMMonitorProvider = RUMMonitorProvider
+        self.RUMMonitorInternalProvider = RUMMonitorInternalProvider


Maybe worth looking into this double injections now that ._internal isn't overly specific.

Because _internal is an extension I cannot override it from a Monitor mock, so I need to inject it as well.

Let me know if you see a good way for doing this :)

I had MockRUMMonitor extend both RUMMonitorProtocol and RUMCommandSubscriber. It's a little white-boxy, but all of the _internal methods send commands to the subscriber, so I check the correct commands get sent to the Subscriber.

Relevant code from the Flutter repo: https://github.com/DataDog/dd-sdk-flutter/blob/79b2c00d4b2fcc81a55093cdbea63f9e3c9d735c/packages/datadog_flutter_plugin/example/ios/Tests/DatadogRumPluginTests.swift#L498

That said, in order for ._internal to be able to return your mock, it requires a change I made in iOS which I don't think is in a released version yet, so... this may be moot.

I believe it's not a huge change to make the change in the tests once your change makes it in a released version, I can make it in a separate PR if that's ok with you?

maciejburda

Looking good. Probably worth upgrading to 2.2.1

Other than that left some minor comments

maciejburda · 2023-09-20T11:16:39Z

packages/core/DatadogSDKReactNative.podspec

+  s.dependency 'DatadogCore', '~> 2.2.0'
+  s.dependency 'DatadogLogs', '~> 2.2.0'
+  s.dependency 'DatadogTrace', '~> 2.2.0'
+  s.dependency 'DatadogRUM', '~> 2.2.0'
+  s.dependency 'DatadogCrashReporting', '~> 2.2.0'
+  s.dependency 'DatadogWebViewTracking', '~> 2.2.0'
+


it's 2.2.1 now :)

maciejburda · 2023-09-20T11:18:17Z

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

+            if (field == null) {
+                if (isInitialized()) {


Does it make sense to use && to join nested if statements in cases like this?

Not sure this leads to a huge improvement to performance, but I think this way conveys more meaning into the order in which we want to make the checks

normally logical checks are performed left to right and if something on the left of && fails it doesn't even compute the right statement.

It's more of a pyramind of doom concern.

maciejburda · 2023-09-20T11:24:28Z

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

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


might be rusty on kotlin, but I think you can avoid force unwrap this way:

configBuilder.setAdditionalConfiguration( additionalConfig?.mapValues { it?.value }.filterNotNull() ?: emptyMap() )

I'd rather make that change in a separate PR to avoid adding more chances to break in this one which is already pretty big if that's ok with you

maciejburda · 2023-09-20T11:25:56Z

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

@@ -7,13 +7,14 @@
 package com.datadog.reactnative

 import android.content.Context
+import com.datadog.android.api.SdkCore
 import com.datadog.android.rum.tracking.ViewTrackingStrategy

 /**
 * No-op implementation of the [ViewTrackingStrategy].
 */
 object NoOpViewTrackingStrategy : ViewTrackingStrategy {


maybe we could make it public 🤔

maciejburda · 2023-09-20T11:28:17Z

packages/core/ios/Sources/DdSdkConfiguration.swift

+    public var uploadFrequency: Datadog.Configuration.UploadFrequency
+    public var batchSize: Datadog.Configuration.BatchSize


we probably should keep defaults for these

I believe these defaults dont make any sense since we always set these values in the init function.
Actually we should probably remove all of them, as they could be misleading and this would allow us to detect if any value is not set by accident in the init function.
Do you agree or do you see a good reason to keep them?

maciejburda · 2023-09-20T11:30:06Z

packages/core/ios/Tests/MockRUMMonitor.swift

+@testable import DatadogInternal
+@testable import DatadogSDKReactNative
+
+internal class MockRUMMonitor: RUMMonitorProtocol {


Are you generating these using stencil or something?

I leverage Sourcery for these kind of tasks. Saves some minutes.

I'm doing it by hand, but I'll improve the toolchain on iOS in a follow-up PR before releasing v2!

maciejburda

LGTM

louiszawadzki force-pushed the louiszawadzki/migrate-ios-sdk-v2 branch 2 times, most recently from c4a3dbf to 375636d Compare September 7, 2023 13:45

louiszawadzki marked this pull request as ready for review September 13, 2023 09:35

louiszawadzki requested a review from a team as a code owner September 13, 2023 09:35

louiszawadzki force-pushed the louiszawadzki/migrate-ios-sdk-v2 branch from 6fd80c4 to 2766f66 Compare September 14, 2023 16:16

fuzzybinary previously approved these changes Sep 15, 2023

View reviewed changes

louiszawadzki dismissed fuzzybinary’s stale review via affc501 September 15, 2023 15:49

louiszawadzki force-pushed the louiszawadzki/migrate-ios-sdk-v2 branch from affc501 to 2766f66 Compare September 19, 2023 13:48

louiszawadzki requested a review from fuzzybinary September 19, 2023 15:18

louiszawadzki added 21 commits September 20, 2023 09:55

Update Android SDK to v2

32ed64e

Extract features enablement to Datadog Wrapper

b066d6c

Fix NoOpViewTrackingStrategy after v2

4534190

Fix Kotlin upgrade errors

94fd15a

Get RUM monitor from DD Wrapper

a951a48

Make applicationId mandatory in android config

2a02208

Fix tests after v2 upgrade

d7a280a

Do not recreate proxies at every call

682369e

Update ktlint

61c9bd4

Get RUM Monitor only once when adding attributes

d2d074f

Rename configuration builders

604e8af

Clean imports in MockRumMonitor

33d3d7d

Migrate iOS SDK to v2

00a7d78

Clarify that info verbosity maps to debug on iOS

00991d3

Fix SDK tests that had changed

cd92cb9

Add tests for telemetry override

9ed755f

Fix timestamp key equality test

21ca029

Update iOS SDK to get full v2 features

f19c35f

Add all fields to iOS configuration telemetry

f0b3ca0

Enable setting custom version on iOS

e89e12c

Reenable ErrorSource iOS tests

86bd6c1

louiszawadzki added 4 commits September 20, 2023 09:59

Reenable ErrorSource iOS tests

511de06

Memoize webview message emitter

eb2eeea

Make new arch example app compile on Android

221fbb2

Make example app compile on Android

53c39fe

louiszawadzki force-pushed the louiszawadzki/migrate-ios-sdk-v2 branch from d16840a to d77f4bb Compare September 20, 2023 07:59

louiszawadzki mentioned this pull request Sep 20, 2023

Migrate Android SDK to v2 #522

Closed

4 tasks

louiszawadzki changed the title ~~Migrate ios sdk to v2~~ Migrate native SDKs to v2 Sep 20, 2023

maciejburda requested changes Sep 20, 2023

View reviewed changes

louiszawadzki added 2 commits September 20, 2023 14:32

Use backed fields for android proxies

b8eeb99

Update iOS SDK to 2.2.1

eee8f69

louiszawadzki force-pushed the louiszawadzki/migrate-ios-sdk-v2 branch from 92f5b5e to eee8f69 Compare September 20, 2023 12:32

Remove force unwrap for attributes

e2ef306

louiszawadzki requested a review from maciejburda September 20, 2023 13:32

maciejburda approved these changes Sep 21, 2023

View reviewed changes

louiszawadzki merged commit 1157f6d into develop Sep 21, 2023
3 checks passed

louiszawadzki deleted the louiszawadzki/migrate-ios-sdk-v2 branch September 21, 2023 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate native SDKs to v2 #523

Migrate native SDKs to v2 #523

louiszawadzki commented Sep 6, 2023 •

edited

Loading

fuzzybinary left a comment

fuzzybinary Sep 15, 2023

fuzzybinary Sep 15, 2023

fuzzybinary Sep 15, 2023

louiszawadzki Sep 15, 2023

maciejburda Sep 20, 2023

fuzzybinary Sep 15, 2023

louiszawadzki Sep 15, 2023

fuzzybinary Sep 15, 2023

louiszawadzki Sep 19, 2023

maciejburda left a comment

maciejburda Sep 20, 2023

maciejburda Sep 20, 2023

louiszawadzki Sep 20, 2023

maciejburda Sep 20, 2023

maciejburda Sep 20, 2023 •

edited

Loading

louiszawadzki Sep 20, 2023

maciejburda Sep 20, 2023

maciejburda Sep 20, 2023

louiszawadzki Sep 20, 2023

maciejburda Sep 20, 2023

louiszawadzki Sep 20, 2023

maciejburda left a comment

		public var uploadFrequency: Datadog.Configuration.UploadFrequency
		public var batchSize: Datadog.Configuration.BatchSize

Migrate native SDKs to v2 #523

Migrate native SDKs to v2 #523

Conversation

louiszawadzki commented Sep 6, 2023 • edited Loading

What does this PR do?

Review checklist (to be filled by reviewers)

fuzzybinary 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

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

maciejburda 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

Choose a reason for hiding this comment

maciejburda Sep 20, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciejburda left a comment

Choose a reason for hiding this comment

louiszawadzki commented Sep 6, 2023 •

edited

Loading

maciejburda Sep 20, 2023 •

edited

Loading