Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow DD initialization from the native side using DdSdkConfiguration #653

Open
ddiachkov opened this issue Apr 11, 2024 · 2 comments
Open
Labels
enhancement New feature or request

Comments

@ddiachkov
Copy link

Feature description

Hello! We have an app with a lot of native code that initializes before the JS side and runs in the background. The native code needs the ability to send logs to DD, and so the SDK must be initialized on application start.

Recently dd-sdk-reactnative added the new DdSdkNativeInitialization.initFromNative API to load configuration from JSON file. Unfortunately, it is a bit limiting since JSON configurations need to be stored in the repo, and it doesn't support environment variables or build environments.

Proposed solution

It would be nice to be able to init dd-sdk-reactnative with just DdSdkConfiguration instance. That would fix the limitations since we can generate it in the runtime using BuildConfig for tokens.

Here is the patch I ended up with:

diff --git a/node_modules/@datadog/mobile-react-native/android/src/main/kotlin/com/datadog/reactnative/DdSdkNativeInitialization.kt b/node_modules/@datadog/mobile-react-native/android/src/main/kotlin/com/datadog/reactnative/DdSdkNativeInitialization.kt
index 1b352a1..d6e6028 100644
--- a/node_modules/@datadog/mobile-react-native/android/src/main/kotlin/com/datadog/reactnative/DdSdkNativeInitialization.kt
+++ b/node_modules/@datadog/mobile-react-native/android/src/main/kotlin/com/datadog/reactnative/DdSdkNativeInitialization.kt
@@ -345,6 +345,24 @@ class DdSdkNativeInitialization internal constructor(
                 )
             }
         }
+
+        /**
+         * Initializes the Datadog React Native SDK using the provided configuration.
+         */
+        @JvmStatic
+        fun initFromNative(appContext: Context, ddSdkConfiguration: DdSdkConfiguration) {
+            val nativeInitialization = DdSdkNativeInitialization(appContext.applicationContext)
+            try {
+                nativeInitialization.initialize(ddSdkConfiguration)
+            } catch (@Suppress("TooGenericExceptionCaught") error: Exception) {
+                Log.w(
+                    DdSdkNativeInitialization::class.java.canonicalName,
+                    "Failed to initialize the Datadog SDK: $error"
+                )
+            }
+        }
     }
 }

Alternatively, making internal constructor public also works.

Other relevant information

For more context, this is how my whole setup works:

  1. On application start, I initialize a new DdSdkConfiguration with settings based on runtime / build environment.
  2. Then I initialize dd-sdk-reactnative via DdSdkNativeInitialization.initFromNative with DdSdkConfiguration.
  3. I also have a native module that provides access to the current DdSdkConfiguration to the JS side (it generates datadog-configuration.json from DdSdkConfiguration).
  4. When JS side starts, I initialize DatadogProvider like this:
const config = new FileBasedConfiguration({
    configuration: NativeModules.Telemetry.datadogConfiguration(),
});

<DatadogProvider configuration={config}>{children}</DatadogProvider>
@ddiachkov ddiachkov added the enhancement New feature or request label Apr 11, 2024
@marco-saia-datadog
Copy link
Member

Hey @ddiachkov, thank you for your great feedback!

I agree that we should allow passing a configuration to the native initialiser, as it can be useful in many different use cases.

We will evaluate this feature soon to work on a solid solution.

At first glance, I believe that we should allow you to retrieve the configuration from our SDK module itself, so that you can use it on the JS side if you have to. On the other hand, I would also consider refactoring the current implementation to avoid having to maintain two configurations, as it would be counterintuitive and possibly unsafe.

We will look into this and keep you updated on this thread.

Thank you again for your contribution!

@kagrawal98
Copy link

+1
it will be really helpful to have this support added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants