- 
                Notifications
    
You must be signed in to change notification settings  - Fork 50
 
[RUM-4912] Configure synthetics attributes from MainActivity intent #702
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
[RUM-4912] Configure synthetics attributes from MainActivity intent #702
Conversation
b3bb207    to
    f953909      
    Compare
  
    | // Workaround for `AdvancedRumMonitor` and `setSyntheticsAttribute` not being visible. | ||
| val intent = Intent().apply { | ||
| putExtra("_dd.synthetics.test_id", DdSdkSynthetics.testId) | ||
| putExtra("_dd.synthetics.result_id", DdSdkSynthetics.resultId) | ||
| } | ||
| 
               | 
          ||
| val proxyActivity = Activity() | ||
| proxyActivity.intent = intent | ||
| 
               | 
          ||
| val lifecycleTracking = object : ActivityLifecycleTrackingStrategy() {} | ||
| val core = DatadogSDKWrapperStorage.getSdkCore() | ||
| lifecycleTracking.register(core, appContext) | ||
| lifecycleTracking.onActivityCreated(proxyActivity, null) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite clear why we are doing this. Synthetics starts the application with intent having necessary attributes and we read them in DdSdk. And then, with any view tracking strategy registered we should be able to read it.
- read in Android SDK https://github.com/DataDog/dd-sdk-android/blob/1ce4aa77e4240fb8b7207f1672e429c2e452de1a/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/tracking/ActivityLifecycleTrackingStrategy.kt#L95-L106
 - view tracking setup in ReactNative SDK 
Lines 123 to 128 in 6d7b1a9
if (configuration.nativeViewTracking == true) { // Use sensible default configBuilder.useViewTrackingStrategy(ActivityViewTrackingStrategy(false)) } else { configBuilder.useViewTrackingStrategy(NoOpViewTrackingStrategy) }  
The only reason why it wouldn't be read is having configuration.nativeViewTracking != true. Is this a "hack" for that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that you are supposed to be calling useViewTrackingStrategy in MainApplication, while on React Native, by the time the initialization is called, the MainActivity has already been created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather then try to do changed in API of Android SDK, so that injection is available from cross-platform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be difficult to achieve, as the method would have to be available before the initialisation of the SDK.
It might be good to have a method that we can call when we register to the lifecycle on DdSdk init, to which we can pass the MainActivity intent so that the Android SDK can internally handle the synthetics attributes setting, or any other operation that has to be performed on the MainActivity.
In my opinion, for the purpose of this PR, it wouldn't be too bad to expose either the setSyntheticsAttribute method or the AdvancedRumMonitor class, so that we can at least make sure that the session type is reported correctly for synthetics.
However, I am open to other ideas, and if we manage to make the View Tracking Strategies work on the MainActivity on RN, I would still expose a basic ViewTrackingStrategy that does not do anything aside from consuming the intent extras for synthetics, so that we can replace the current default NoOpViewTrackingStrategy.
88bf7ba    to
    fb7ac6c      
    Compare
  
    fb7ac6c    to
    bf1c3eb      
    Compare
  
    57d5f85    to
    37f75e6      
    Compare
  
    bf1c3eb    to
    a69b247      
    Compare
  
    a69b247    to
    cd86b6b      
    Compare
  
    ff1b34a
      into
      
  
    marcosaia/RUM-5918/update-dd-sdks
  
    
What does this PR do?
Retrieves synthetics attributes from the MainActivity intent and configures RUM, so that the session will be detected, treated and reported as a synthetics test.
Context
In Android SDK, synthetics attributes (
result_idandtest_id) are retrieved from the intent extras inActivityLifecycleTrackingStrategy.ActivityLifecycleTrackingStrategyis a superclass of different View Tracking strategies that can be enabled throughRumConfigurationBuilderby using.useViewTrackingStrategy.In React Native SDK, we don't enable native views tracking unless it is explicitly specified by the users in the SDK configuration and - more importantly - we don't initialise the SDK in application scope, and the intent extras are lost for this reason.
Proposed Solution
We can register to the lifecycle events using the application context when the RN module is initialized, using
context.addLifecycleEventListener, retrieve the intent extras, and use them later during the SDK initialization.Problem
We currently do not have a way to set the synthetics attributes, because the way its done on Android is through
AdvancedRumMonitor, which is an internal class and it is not visible from RN.Code from ActivityLifecycleTrackingStrategy.kt:
Workaround
This PR introduces a workaround to still take advantage of the Android code, and demonstrate that we can in fact intercept and set the intent extras using the lifecycle event listener.
The workaround consists in manually calling the
ActivityLifecycleTrackingStrategymethods with a dummy activity that contains the required intent extras:Conclusion
I believe we have to expose a method from Android SDK that allows cross platform frameworks to set the synthetics attributes, and get rid of this workaround.
Review checklist (to be filled by reviewers)