RUM-5843 Introduce the RumMonitor#addViewLoadingTime API by mariusc83 · Pull Request #2243 · DataDog/dd-sdk-android

mariusc83 · 2024-09-05T13:53:52Z

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
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)

codecov-commenter · 2024-09-05T15:47:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.09%. Comparing base (d059818) to head (02d3565).
Report is 7 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2243      +/-   ##
===========================================
+ Coverage    70.05%   70.09%   +0.04%     
===========================================
  Files          730      730              
  Lines        27189    27185       -4     
  Branches      4590     4594       +4     
===========================================
+ Hits         19045    19053       +8     
+ Misses        6842     6838       -4     
+ Partials      1302     1294       -8

Files with missing lines	Coverage Δ
.../main/kotlin/com/datadog/android/rum/RumMonitor.kt	`28.57% <ø> (ø)`
...g/android/rum/internal/domain/scope/RumRawEvent.kt	`100.00% <100.00%> (ø)`
.../android/rum/internal/domain/scope/RumViewScope.kt	`94.63% <100.00%> (-0.65%)`	⬇️
.../android/rum/internal/monitor/DatadogRumMonitor.kt	`87.32% <100.00%> (-0.25%)`	⬇️

... and 30 files with indirect coverage changes

0xnm · 2024-09-06T07:41:56Z


+    /**
+     * Adds view loading time to current RUM view based on the time elapsed since the view was started.
+     * This method should be called only once per view.


Why do we enforce it requirement on the client side? Cannot we just check in the implementation that if it was called already for the view, then ignore subsequent calls?

we follow same approach as on iOS and for now on their end they allow it to be called multiple times. @ganeshnj maybe you want to add some thoughts here ?

Important
I agree with @0xnm , we can't ask the customer to know whether or not the method has already been called. They could have several code paths that would lead to calling this method, and it would be a hassle to ask them to track whether or not that method have been called for a given View.
We need to make our SDK resilient enough to handle "misuse".

0xnm · 2024-09-06T07:46:06Z

+
+        // Then
+        argumentCaptor<ViewEvent> {
+            verify(mockWriter, times(viewLoadingTimeEvents.size))


why should we allow multiple calls?

same answer as above.

Note
Not sure what the RFC stated, but imo the first loading time is the one that should be kept, otherwise, if the same screen is updated/refreshed, we would report a longer loading time than what the end user experienced.

0xnm · 2024-09-06T07:47:09Z

        datadogBenchmark.stop()
    }

+    @OptIn(ExperimentalRumApi::class)


It seems it is not needed here? I don't see any addViewLoadingTime call here

yup a left - over from previous tries :(

0xnm · 2024-09-06T07:51:02Z

+    @OptIn(ExperimentalRumApi::class)
+    override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) {
+        if (event == Lifecycle.Event.ON_RESUME) {
+            GlobalRumMonitor.get().addViewLoadingTime()


won't we have multiple calls for the same view here when application goes info background and then back into foreground?

when the app goes into background there will be a new view...we stop the view on the onPause the way I remember.

no, here view tracking relies on navigation - I don't think navigation events are fired when app changes the state (but I may be wrong). While addViewLoadingTime call here relies not on the navigation, but on the lifecycle events.

let me double check this, you might be right, I was assuming we rely on the view lifecycle here

0xnm · 2024-09-06T07:51:40Z

+    @OptIn(ExperimentalRumApi::class)
+    override fun onResume() {
+        super.onResume()
+        GlobalRumMonitor.get().addViewLoadingTime()


same: I guess we can have multiple calls for the same view?

same as above

0xnm · 2024-09-06T07:52:31Z

+    @OptIn(ExperimentalRumApi::class)
+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
+        super.onViewCreated(view, savedInstanceState)
+        GlobalRumMonitor.get().addViewLoadingTime()


onResume will be called right after onViewCreated effectively discarding this call

0xnm · 2024-09-06T07:53:42Z

            object : ImageLoadedCallback {
                override fun onImageLoaded(resource: Drawable) {
                    imageViewRemote.setImageDrawable(resource)
+                    if (imageLoadedCounter.decrementAndGet() == 0) {


should we extract this into a common private method? same snippet is used 4 times.

0xnm · 2024-09-06T07:54:28Z

    )

+    @Volatile
+    private var pageWasLoaded: Boolean = false


Suggested change

private var pageWasLoaded: Boolean = false

private var pageIsLoaded: Boolean = false

I know that the WebView callbacks are being executed on a different thread this is the reason why I want to keep it as @volatile

my suggestion is not about volatile, but about naming

0xnm · 2024-09-06T07:54:47Z

    private lateinit var webView: WebView

+    @Volatile
+    private var pageWasLoaded: Boolean = false


Suggested change

private var pageWasLoaded: Boolean = false

private var pageIsLoaded: Boolean = false

0xnm · 2024-09-06T07:55:30Z

    )

+    @Volatile
+    private var pageWasLoaded: Boolean = false


Suggested change

private var pageWasLoaded: Boolean = false

private var pageIsLoaded: Boolean = false

same as above

xgouchet · 2024-09-06T08:45:01Z

+     * If the view is not started, this method does nothing.
+     * If the view is not active(stopped), this method does nothing.


Note
Since the loading time is applied to the current RUM view (maybe should be the current active RUM View), those two lines are misleading IMO. It feels like we make the assumption that a view is necessarily an Activity/Fragment with a lifecycle (start, resume, …), which is not always the case.

Suggested change

* If the view is not started, this method does nothing.

* If the view is not active(stopped), this method does nothing.

* If no view is started or active, this method does nothing.

I am following the same docs as in iOS

xgouchet · 2024-09-06T08:47:16Z


+    /**
+     * Adds view loading time to current RUM view based on the time elapsed since the view was started.
+     * This method should be called only once per view.


Important
I agree with @0xnm , we can't ask the customer to know whether or not the method has already been called. They could have several code paths that would lead to calling this method, and it would be a hassle to ask them to track whether or not that method have been called for a given View.
We need to make our SDK resilient enough to handle "misuse".

xgouchet · 2024-09-06T08:49:43Z

+
+        // Then
+        argumentCaptor<ViewEvent> {
+            verify(mockWriter, times(viewLoadingTimeEvents.size))


Note
Not sure what the RFC stated, but imo the first loading time is the one that should be kept, otherwise, if the same screen is updated/refreshed, we would report a longer loading time than what the end user experienced.

xgouchet · 2024-09-06T08:55:21Z

+
+    @OptIn(ExperimentalRumApi::class)
+    override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) {
+        if (event == Lifecycle.Event.ON_RESUME && !viewIsLoaded) {


Note
Is this sample the kind of use case we want to showcase ? Tracking the onResume time is not relevant most of the time. We should probably instead only do what you added in the DataListViewModel where we actually load something from the network, which would be a better example of how to use this new API.

For some reason I cannot reply to your comment one by one @xgouchet. Regarding the addViewLoadingTime being called multiple times the whole approach was discussed and agreed in the RFC.

Regarding your last comment, that is a nice to have in the Sample app, my intention was to add the viewLoadingTime for most of the views there. In that case I will remove the call for this view and maybe add a task to enrich the sample app. This is not the scope for this PR. @0xnm what's your opinion on that ?

It depends on the nature of the screen, some screens may not load anything from the network/disk. But maybe to highlight that this method shouldn't be blindly used in the onResume we may want indeed to have it called only in the data stream.

On the same time we could agree that there will be use cases on the customer's end where simple static views like these ones would have to be measured. In that case we will remove these use - cases from the sample app and only maintain the ones that imply an async data ?

xgouchet · 2024-09-11T06:49:47Z

        startCustomRumViewButton.setOnClickListener {
            GlobalRumMonitor.get().startView(this, "Custom RUM View")
        }
+        // add webview on load listener


Note
Is this comment needed here ? Feels like its a TODO that didn't get removed

mariusc83 self-assigned this Sep 5, 2024

mariusc83 force-pushed the mconstantin/rum-5843/implement-addviewloadingtime-api branch 2 times, most recently from 8857cb3 to 51443c5 Compare September 5, 2024 15:18

mariusc83 marked this pull request as ready for review September 6, 2024 06:57

mariusc83 requested review from a team as code owners September 6, 2024 06:57

0xnm reviewed Sep 6, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-5843/implement-addviewloadingtime-api branch from 51443c5 to fe279fd Compare September 6, 2024 08:54

xgouchet reviewed Sep 6, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-5843/implement-addviewloadingtime-api branch 2 times, most recently from 2b818a3 to 13199cd Compare September 9, 2024 13:20

mariusc83 requested review from 0xnm and xgouchet September 9, 2024 14:22

mariusc83 force-pushed the mconstantin/rum-5843/implement-addviewloadingtime-api branch from 13199cd to 61ca355 Compare September 10, 2024 08:06

xgouchet approved these changes Sep 11, 2024

View reviewed changes

mariusc83 added 5 commits September 13, 2024 11:53

Have the NoOpGenerator support the RequiredOptIn functions

bb89022

Introduce the RumMonitor#addViewLoadingTimeApi

8f5c1b2

Use the RumMonitor#addViewLoadingTIme API into the sample app

aefa556

Modify the addViewLoadingTime API according with new RFC

98357f2

Modify the sample app based on latest changes on addViewLoadingTime API

02d3565

mariusc83 force-pushed the mconstantin/rum-5843/implement-addviewloadingtime-api branch from 61ca355 to 02d3565 Compare September 13, 2024 08:53

mariusc83 merged commit febadea into develop Sep 13, 2024

mariusc83 deleted the mconstantin/rum-5843/implement-addviewloadingtime-api branch September 13, 2024 09:31

	private var pageWasLoaded: Boolean = false
	private var pageIsLoaded: Boolean = false

		* If the view is not started, this method does nothing.
		* If the view is not active(stopped), this method does nothing.

	* If the view is not started, this method does nothing.
	* If the view is not active(stopped), this method does nothing.
	* If no view is started or active, this method does nothing.

Uh oh!

Conversation

mariusc83 commented Sep 5, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov-commenter commented Sep 5, 2024 •

edited

Loading