Skip to content

RUM-7174 Introduce the setNetworkSettledInitialResourceIdentifier API#2424

Merged
mariusc83 merged 2 commits into
feature/view-loading-timesfrom
mconstantin/rum-7174/add-timing-strategy-for-ttns-requests-identification
Dec 4, 2024
Merged

RUM-7174 Introduce the setNetworkSettledInitialResourceIdentifier API#2424
mariusc83 merged 2 commits into
feature/view-loading-timesfrom
mconstantin/rum-7174/add-timing-strategy-for-ttns-requests-identification

Conversation

@mariusc83

@mariusc83 mariusc83 commented Dec 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

Following the RFC we are introducing the setNetworkSettledInitialResourceIdentifier API as part of the RumConfiguration#Builder in order to provide external control on which resource started in a view will be take part
in the time-to-network-settled metric computation. The default identifier being used is a TimeBasedResourceIdentifier with a default threshold set to 100ms. This could be re - used by our users in the Configuration Builder by changing the time interval to their internal needs.

In addition to this this PR contain a small but very important fix in the RumViewScope where we make sure the NetworkSettledMetricResolver is notified for the view stopped only if the event was sent for the current view and only once.

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)

@mariusc83 mariusc83 self-assigned this Dec 2, 2024
@mariusc83
mariusc83 force-pushed the mconstantin/rum-7174/add-timing-strategy-for-ttns-requests-identification branch from 8f6c29d to dd94ef2 Compare December 2, 2024 14:29
@mariusc83 mariusc83 changed the title RUM-7174 Introduce the setNetworkSettledInitialResourceIdentifier Pub… RUM-7174 Introduce the setNetworkSettledInitialResourceIdentifier API Dec 2, 2024
@mariusc83
mariusc83 force-pushed the mconstantin/rum-7174/add-timing-strategy-for-ttns-requests-identification branch from dd94ef2 to a151145 Compare December 2, 2024 14:52
@mariusc83
mariusc83 marked this pull request as ready for review December 2, 2024 14:56
@mariusc83
mariusc83 requested review from a team as code owners December 2, 2024 14:56
@codecov-commenter

codecov-commenter commented Dec 2, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 98.07692% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.10%. Comparing base (614e7c0) to head (48ab0a2).
Report is 3 commits behind head on feature/view-loading-times.

Files with missing lines Patch % Lines
...tworksettled/TimeBasedInitialResourceIdentifier.kt 90.91% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@                      Coverage Diff                       @@
##           feature/view-loading-times    #2424      +/-   ##
==============================================================
+ Coverage                       69.96%   70.10%   +0.14%     
==============================================================
  Files                             775      775              
  Lines                           28647    28676      +29     
  Branches                         4805     4807       +2     
==============================================================
+ Hits                            20042    20103      +61     
+ Misses                           7277     7249      -28     
+ Partials                         1328     1324       -4
Files with missing lines Coverage Δ
...rum/src/main/kotlin/com/datadog/android/rum/Rum.kt 92.31% <100.00%> (+0.15%) ⬆️
...kotlin/com/datadog/android/rum/RumConfiguration.kt 96.92% <100.00%> (+0.10%) ⬆️
...lin/com/datadog/android/rum/internal/RumFeature.kt 92.64% <100.00%> (+0.10%) ⬆️
...d/rum/internal/domain/scope/RumApplicationScope.kt 94.85% <100.00%> (+0.16%) ⬆️
...droid/rum/internal/domain/scope/RumSessionScope.kt 96.67% <100.00%> (+0.03%) ⬆️
...d/rum/internal/domain/scope/RumViewManagerScope.kt 91.95% <100.00%> (+0.39%) ⬆️
.../android/rum/internal/domain/scope/RumViewScope.kt 95.69% <100.00%> (+0.93%) ⬆️
...ric/networksettled/NetworkSettledMetricResolver.kt 97.37% <ø> (ø)
.../android/rum/internal/monitor/DatadogRumMonitor.kt 85.57% <100.00%> (+0.43%) ⬆️
...ic/networksettled/NetworkSettledResourceContext.kt 100.00% <100.00%> (ø)
... and 1 more

... and 25 files with indirect coverage changes

xgouchet
xgouchet previously approved these changes Dec 3, 2024
View reviewed changes
Comment thread features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt Outdated
* a threshold of 100ms.
* @param initialResourceIdentifier the [InitialResourceIdentifier] to use.
*/
fun setNetworkSettledInitialResourceIdentifier(initialResourceIdentifier: InitialResourceIdentifier): Builder {

@xgouchet xgouchet Dec 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This function's name is a bit long, maybe we can rename it to setInitialResourceIdentifier?

@mariusc83 mariusc83 Dec 4, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad that you mentioned it...was also thinking about it but I was afraid that only using setInitialResourceIdentifier will not be explicit enough. I guess we are ok though.

Comment thread features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/RumFeature.kt Outdated
private var anrDetectorExecutorService: ExecutorService? = null
internal var anrDetectorRunnable: ANRDetectorRunnable? = null
internal lateinit var appContext: Context
internal var networkSettledInitialResourceIdentifier: InitialResourceIdentifier = NoOpInitialResourceIdentifier()

@xgouchet xgouchet Dec 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Same, maybe we can rename this property to initialResourceIdentifier.

Comment thread ...oid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumApplicationScope.kt Outdated
private val sessionEndedMetricDispatcher: SessionMetricDispatcher,
private val sessionListener: RumSessionListener?
private val sessionListener: RumSessionListener?,
private val networkSettledResourceIdentifier: InitialResourceIdentifier

@xgouchet xgouchet Dec 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Same, maybe we can rename this property to initialResourceIdentifier.

Comment thread ...android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumSessionScope.kt
frameRateVitalMonitor: VitalMonitor,
private val sessionListener: RumSessionListener?,
applicationDisplayed: Boolean,
networkSettledResourceIdentifier: InitialResourceIdentifier,

@xgouchet xgouchet Dec 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Same, maybe we can rename this property to initialResourceIdentifier.

Comment thread ...oid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewManagerScope.kt Outdated
private val frameRateVitalMonitor: VitalMonitor,
internal var applicationDisplayed: Boolean,
internal val sampleRate: Float,
internal val networkSettledResourceIdentifier: InitialResourceIdentifier,

@xgouchet xgouchet Dec 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Same, maybe we can rename this property to initialResourceIdentifier.

Comment thread ...dk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt
private val interactionToNextViewMetricResolver: InteractionToNextViewMetricResolver,
private val networkSettledMetricResolver: NetworkSettledMetricResolver =
NetworkSettledMetricResolver(internalLogger = sdkCore.internalLogger)
private val networkSettledMetricResolver: NetworkSettledMetricResolver

@xgouchet xgouchet Dec 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Same, maybe we can rename this property to initialResourceIdentifier.

Comment thread ...dk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt Outdated
sessionListener: RumSessionListener,
internal val executorService: ExecutorService
internal val executorService: ExecutorService,
internal val networkSettledResourceIdentifier: InitialResourceIdentifier

@xgouchet xgouchet Dec 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Same, maybe we can rename this property to initialResourceIdentifier.

ambushwork
ambushwork previously approved these changes Dec 3, 2024
View reviewed changes
@mariusc83
mariusc83 dismissed stale reviews from ambushwork and xgouchet via 48ab0a2 December 4, 2024 09:15
@mariusc83
mariusc83 force-pushed the mconstantin/rum-7174/add-timing-strategy-for-ttns-requests-identification branch from 85893e3 to 48ab0a2 Compare December 4, 2024 09:15
@mariusc83
mariusc83 merged commit ce81b20 into feature/view-loading-times Dec 4, 2024
@mariusc83
mariusc83 deleted the mconstantin/rum-7174/add-timing-strategy-for-ttns-requests-identification branch December 4, 2024 12:55
mariusc83 added a commit that referenced this pull request Dec 4, 2024
@mariusc83
Merge pull request #2424 from DataDog/mconstantin/rum-7174/add-timing…
2b7e39a 
…-strategy-for-ttns-requests-identification

RUM-7174 Introduce the setNetworkSettledInitialResourceIdentifier API
mariusc83 added a commit that referenced this pull request Dec 4, 2024
@mariusc83
Merge pull request #2424 from DataDog/mconstantin/rum-7174/add-timing…
2acc626 
…-strategy-for-ttns-requests-identification

RUM-7174 Introduce the setNetworkSettledInitialResourceIdentifier API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ambushwork ambushwork ambushwork left review comments
+1 more reviewer
@xgouchet xgouchet xgouchet approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mariusc83 mariusc83

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mariusc83 @codecov-commenter @xgouchet @ambushwork