-
Notifications
You must be signed in to change notification settings - Fork 76
RUM-12062: Refactor the configuration and integration of the RUM Debug Widget #2975
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
base: tvaleev/feature/innovation-week-07-04-25
Are you sure you want to change the base?
Conversation
…Initializer and AutoStarter
…n LICENSE 3rd party
…/change-widget-activity-configuration
|
🎯 Code Coverage 🔗 Commit SHA: 7c2f184 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
…a/RUM-12062/change-widget-activity-configuration
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.
Pull Request Overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...dget/src/main/kotlin/com/datadog/android/insights/internal/overlay/DefaultInsightsOverlay.kt
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumDebugWidget.kt
Show resolved
Hide resolved
...debug-widget/src/main/kotlin/com/datadog/android/insights/internal/overlay/OverlayManager.kt
Show resolved
Hide resolved
...dget/src/main/kotlin/com/datadog/android/insights/internal/overlay/DefaultInsightsOverlay.kt
Show resolved
Hide resolved
...dget/src/main/kotlin/com/datadog/android/insights/internal/overlay/DefaultInsightsOverlay.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| root?.let { v -> | ||
| if (v.parent != parent) { |
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.
Why is this if needed? If I understand correctly it is always false, because we only set root using the code above:
val overlayView = activity.layoutInflater.inflate(
R.layout.layout_dd_insights_overlay,
parent,
false
)
root = overlayView
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.
This is false for the first call to attach(), but for the next calls it could be true when moving the overlay between activities, so we should check to prevent reattaching, for example.
...dget/src/main/kotlin/com/datadog/android/insights/internal/overlay/DefaultInsightsOverlay.kt
Show resolved
Hide resolved
...dget/src/main/kotlin/com/datadog/android/insights/internal/overlay/DefaultInsightsOverlay.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Outdated
Show resolved
Hide resolved
|
|
||
| @Suppress("UnsafeThirdPartyFunctionCall") | ||
| runCatching { | ||
| val clazz = Class.forName("com.datadog.android.insights.internal.RumDebugWidget") |
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.
What do you think of the following option to avoid reflection?
setInsightsCollectoris in_RumInternalProxy. https://github.com/DataDog/dd-sdk-android/pull/2975/files#r2494746331.- Then in this
enableRumDebugWidgetyou call:
val insightsCollector = DefaultInsightsCollector()
_RumInternalProxy.setInsightsCollector(builder, insightsCollector)
OverlayManager(insightsCollector).start(application)
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.
upd
Sorry, forgot an important part. This file should be moved to dd-sdk-android-rum-debug-widget
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.
All this reflection inside the RUM module is because the rum-debug-widget module is supposed to only be included in debug builds, by using debugImplementation in the build.gradle file. Otherwise yes, we would put this extension to RumConfiguration.Builder in the rum-debug-widget module, but we want to safeguard the customer from crashing the app in production.
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.
All this reflection inside the RUM module is because the rum-debug-widget module is supposed to only be included in debug builds, by using debugImplementation in the build.gradle file.
If the customers want to use debugImplementation("dd-sdk-android-rum-debug-widget"), they can create a sourceSet for debug and call enableRumDebugWidget from there.
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.
But in that case you will have to keep another copy of Application class there. Don you think it might be too complex in general for the customers so they gonna skip that option and add widget as a dependency directly in the app ending with it in a production build ?
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.
As discussed with @satween, we can keep the reflection and don't need the consumer/proguard rules to clean this code. The refactoring on the enableRumDebugWidget function handles this well (as also explained in this comment).
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.
and don't need the consumer/proguard rules to clean this code
consumer-rules.pro are needed not for cleaning, but so that the code of dd-sdk-android-rum-debug-widget isn't removed from the application by R8. See this example
16215ba. You can run sample:kotlin in debug (I enabled R8 there to demonstrate) and see that RumDebugWidget.enable isn't called.
RumDebugWidget was removed from the APK of the sample app by R8.
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.
You are right, added the consumer-rules.pro file.
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 rum-debug-widget module is supposed to only be included in debug builds
but at the same time we have allowInRelease argument in this method.
Overall, it is a debug tool which may be used in the release build to verify SDK integration (it makes sense if we merge the functionality of RumMonitor.debug here, because customers may want to check RUM view names in release builds if they are based on the class names).
I think we simply may want to take integration approach used by LeakCanary, I guess there it is closer to what @aleksandr-gringauz is proposing?
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 think we simply may want to take integration approach used by LeakCanary, I guess there it is closer to what @aleksandr-gringauz is proposing?
I suppose yes, it is similar:
- In the docs it suggests you use debugImplementation.
- Here there is an example of doing custom configuration in debug sourceSet.
One thing to mention is that custom configuration is optional, LeakCanary will start doing something just when you add the dependency. It does that with the help of multiple ContentProviders link. I'm not suggesting doing the same thing in our case (we don't need it), just exploring what they are doing.
...debug-widget/src/main/kotlin/com/datadog/android/insights/internal/overlay/OverlayManager.kt
Outdated
Show resolved
Hide resolved
...bug-widget/src/main/kotlin/com/datadog/android/insights/internal/DefaultInsightsCollector.kt
Outdated
Show resolved
Hide resolved
| */ | ||
| fun RumConfiguration.Builder.enableRumDebugWidget( | ||
| application: Application, | ||
| enabled: Boolean = application.isDebuggable() |
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.
IMHO this enabled parameter is useless. The method is called enableRumDebugWidget. If the customer calls it, they want to enable it.
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 idea here, and as dicussed with @satween, is that we wanted to safeguard the customer from accidentally enabling this in release builds. For this, they would have to explicitly pass true in the enabled parameter.
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.
Well the idea was to use Build.DEBUG constant, not the application.isDebuggable().
Let's try to remove the enabled parameter and put whole logic into the if(BuildConfig.DEBUG) scope as suggested by @aleksandr-gringauz in the comment above
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.
if(BuildConfig.DEBUG)
I don't understand how this will work. Each gradle module has its own BuildConfig class and it is a statically generated thing. BuildConfig.DEBUG will be equal to false in the published artifacts of all modules.
What I was talking about is if(BuildConfig.DEBUG) where BuildConfig is from the app module of the customer.
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 don't understand how this will work. Each gradle module has its own BuildConfig class and it is a statically
generated thing. BuildConfig.DEBUG will be equal to false in the published artifacts of all modules.
I thought there is some R8 feature that I missed.
Let me explain the problem we are trying to solve here: We want to let customer use dd-android-sdk-rum-widget module for debug the Datadog SDK integration. We are expecting that the widget is going to be used only for the debug purposes, but in the other hand - want to make it handy and easy to use.
From that perspective we want to avoid:
- Accidentally adding widget into release build of the customer's app (at the first glance it could be solved with the
debugImplementation) - Add the additional effort to manually remove usages of the widget from the release build. In case if customer will add widget as a
debugImplementationdependency, then use it in the code - it will break the compilation of the release build.
So I see several options to do so:
- Create additional
debugsource on the customer's side - but this could be hard for some projects. Debug version of the Application has to be maintained and seems overkill for just a widget. - Use reflection to check if widget is added as a dependency in the runtime.
To reduce the impact of the reflection to the application launch time we could ask customer to provide BuildConfig.DEBUG value with the enabled parameter to not call reflection in the production.
WDYT ?
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.
To reduce the impact of the reflection to the application launch time we could ask customer to provide BuildConfig.DEBUG value with the enabled parameter to not call reflection in the production.
IIUC you are proposing the following API:
fun RumConfiguration.Builder.enableRumDebugWidget(
application: Application,
enabled: Boolean
)
And in the documentation we tell the customers that they can pass BuildConfig.DEBUG as enabled parameter.
Ok, this will work.
To me 2 "enable" words in one signature look confusing, I would probably call it setRumDebugWidgetEnabled(enabled) or if we want to call enableRumDebugWidget - just drop the enabled argument.
This is why I initially started this particular thread about enabled argument.
Not blocking the PR.
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 changed the argument enabled to allowInRelease which defaults to false. This safeguards customers from accidentally enabling this in production, where the reflection would run with no success, as the dd-sdk-android-rum-debug-widget module should be added as debugImplementation in the build.gradle file. To really use the widget in release builds (e.g. local testing), customers should set allowInRelease to true and add the module as implementation. This is well explained in the function documentation.
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/_RumInternalProxy.kt
Outdated
Show resolved
Hide resolved
...debug-widget/src/main/kotlin/com/datadog/android/insights/internal/overlay/OverlayManager.kt
Outdated
Show resolved
Hide resolved
| private var isTransitioning: Boolean = false | ||
|
|
||
| @Suppress("LongMethod") | ||
| internal fun attach(activity: Activity) { |
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.
Not blocking for this PR, just some thoughts.
The current API for the widget integration allows either to enable it so that is visible everywhere in the app or not enable it at all. Also it is impossible to programmatically hide it.
Big apps might have some kind of Debug view that contains things related to debugging. For example it might contain toggles for feature flags, some debugging tools specific for the app. In this case it would be convenient to be able to enable and disable our widget by clicking on some button there. But currently it is impossible.
Also apps might want to attach the view of the widget not to the root view of activity, but somewhere else (maybe some debug panel that can be swiped to the side?).
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 idea behind this widget is that the customers could enable it while they work on SDK integration and then remove it completley. This is why I was thinking about adding it only as a debugImplentation dependency in order to prevent widget being available in release build of the app. Having an option to add this widget somewhere beside the root of activity a good idea in general, but would require much more effort to support (like the case with an app using only compose ui) and goes beside the scope of this initiative.
We just want to provide faster feedback loop for the developers that struggle to integrate the SDK.
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 idea behind this widget is that the customers could enable it while they work on SDK integration and then remove it completley.
We just want to provide faster feedback loop for the developers that struggle to integrate the SDK.
So it isn't a widget for investigating application performance (it's memory usage, skipped frames, other things) but rather a widget for debugging sdk integration?
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.
Yes, widget is for:
- Validating the integration (e.g see if the SDK identifies the user actions correctly)
- Debugging some issues like memory leaks or high CPU usage (but only in dev environment)
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.
Not blocking the PR, just some ideas.
Debugging some issues like memory leaks or high CPU usage (but only in dev environment)
It could useful to be able to do it in some builds that are intended for QA-engineers or the builds that are used internally in the customer's company. These are usually release builds and are often implemented as a separate Gradle flavor.
Also performance issues are often investigated in release, because when you enable compiler optimizations the picture usually changes.
Having an option to add this widget somewhere beside the root of activity a good idea in general, but would require much more effort to support (like the case with an app using only compose ui) and goes beside the scope of this initiative.
The way it could be done is that we provide a custom android view (implemented in Android Views, not compose) as a public API. And inside it do something like GlobalRumMonitor.getInstance(sdkName).getInsightsCollector.subscribe { data -> updateWidget(data) } in this CustomView.onAttachedToWindow. sdkName should be provided by the customer's code either through a view's constructor or through a custom xml tag. Apps with Compose will be able to use it like this.
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.
Yes, widget is for:
Validating the integration (e.g see if the SDK identifies the user actions correctly)
Then we have a to think what to do with RumMonitor.debug, because its purpose is also to debug SDK integration.
With this change we will have two "RUM debug" entrypoints.
Should we maybe unify things? WDYT?
| @Suppress("OPT_IN_USAGE") | ||
| internal object RumDebugWidget { | ||
|
|
||
| @JvmStatic |
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.
Is this needed, given that class is internal, so this method can be only called from Kotlin? Or is it to simplify reflection somehow?
| import com.datadog.android.rum.RumConfiguration | ||
| import com.datadog.android.rum._RumInternalProxy | ||
|
|
||
| @Suppress("OPT_IN_USAGE") |
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.
is it needed?
|
|
||
| @Test | ||
| fun `M use NoOpInsightsCollector W setInsightsCollector(null)`() { | ||
| fun `M use NoOpInsightsCollector W don't setInsightsCollector()`() { |
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.
| fun `M use NoOpInsightsCollector W don't setInsightsCollector()`() { | |
| fun `M use NoOpInsightsCollector W build() { default configuration }`() { |
| private var isTransitioning: Boolean = false | ||
|
|
||
| @Suppress("LongMethod") | ||
| internal fun attach(activity: Activity) { |
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.
Yes, widget is for:
Validating the integration (e.g see if the SDK identifies the user actions correctly)
Then we have a to think what to do with RumMonitor.debug, because its purpose is also to debug SDK integration.
With this change we will have two "RUM debug" entrypoints.
Should we maybe unify things? WDYT?
| * to prevent accidental exposure in production. | ||
| * @return The same [RumConfiguration.Builder] instance. | ||
| */ | ||
| fun RumConfiguration.Builder.enableRumDebugWidget( |
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.
why it is an extension method rather than class method?
|
|
||
| @Suppress("UnsafeThirdPartyFunctionCall") | ||
| runCatching { | ||
| val clazz = Class.forName("com.datadog.android.insights.internal.RumDebugWidget") |
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 rum-debug-widget module is supposed to only be included in debug builds
but at the same time we have allowInRelease argument in this method.
Overall, it is a debug tool which may be used in the release build to verify SDK integration (it makes sense if we merge the functionality of RumMonitor.debug here, because customers may want to check RUM view names in release builds if they are based on the class names).
I think we simply may want to take integration approach used by LeakCanary, I guess there it is closer to what @aleksandr-gringauz is proposing?
What does this PR do?
enableRumDebugWidget()in theirRumConfiguration.Builder.README.md.OverlayManagerusing activity lifecycle callbacks.Motivation
Configuration was too complex for customers, where some unnecessary APIs had to be exposed. The
DefaultInsightsOverlayhad to be manually attached to every activity by customers.Review checklist (to be filled by reviewers)