-
Notifications
You must be signed in to change notification settings - Fork 136
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-7102 feat: Add Time To Network Settled metric in RUM #2125
RUM-7102 feat: Add Time To Network Settled metric in RUM #2125
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 3617 Passed, 0 Skipped, 2m 21.76s Total Time 🔻 Code Coverage Decreases vs Default Branch (5) |
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 done! 💪 Just a couple of non-blocking questions and comments, but overall, it's nicely written, well-documented, and the test suite is robust.
@@ -1827,6 +1830,7 @@ public struct RUMResourceEvent: RUMDataModel { | |||
case transferSize = "transfer_size" | |||
case type = "type" | |||
case url = "url" | |||
case worker = "worker" |
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.
Do you know if the explicit key mappings are required to match a backend schema?
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.
They are required as schema mixes multiple conventions (mainly snake_case and kebab-case), e.g.:
case analytics = "analytics"
// ...
case reactNative = "react-native"
// ...
case sessionSampleRate = "session_sample_rate"
/// - endDate: The end time of the resource (device time, no NTP offset). | ||
/// - resourceID: The unique identifier for the resource. | ||
/// - resourceDuration: The resource duration, if available. | ||
func trackResourceEnd(at endDate: Date, resourceID: RUMUUID, resourceDuration: TimeInterval?) { |
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 resourceDuration
provided at all? Shouldn't the method rely solely on endDate
to calculate the duration? I can see that in the RUMResourceScope
we provide it when we send a resource event, but not when tracking an error.
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 "start" and "end" times for RUM resource are approximations based on network request timings derived from the resume()
and completion {}
in URLSession.
For a more accurate representation, RUM listens to URLSessionTaskMetrics
delivered to the session delegate. In addition to various temporal metrics such as DNS, TCP, and TLS timings, URLSessionTaskMetrics
also provides the fetch duration. This fetch duration serves as a more precise alternative for the RUM resource "duration," which is now tracked here for the TTNS metric.
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.
Got it, thanks for the explanation!
@@ -2595,7 +2595,7 @@ class RUMViewScopeTests: XCTestCase { | |||
XCTAssertEqual(event.view.resource.count, 1, "After dropping 1 Resource event (out of 2), View should record 1 Resource") | |||
XCTAssertEqual(event.view.action.count, 0, "After dropping a User Action event, View should record no actions") | |||
XCTAssertEqual(event.view.error.count, 0, "After dropping an Error event, View should record 0 Errors") | |||
XCTAssertEqual(event.dd.documentVersion, 3, "After starting the application, stopping the view, starting/stopping one resource out of 2, discarding a user action and an error, the View scope should have sent 3 View events.") | |||
XCTAssertEqual(event.dd.documentVersion, 4, "It should create 4 view update.") |
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 expected as now we also cause view updates when events are dropped. This doesn't change what is sent to RUM intake as redundant updates are filtered before upload.
@mariedm Thanks for review! All feedback addressed 👌. I'm requesting another approval because there was small addition of new logic in the last commit (fd4e916) following recent spec changes. We now only count resources that are sent to RUM, excluding any resources that are dropped by the user with event mapper API. |
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.
LGTM! 🚀
What and why?
📦⏱️ This PR introduces the Time-to-Network-Settled (TTNS) metric to measure the time from when a view becomes visible until all initial network resources have finished loading. This metric helps gauge the time it takes for a view to be fully displayed with all required resources.
The TTNS value is reported under these conditions:
Example: In following scenario, resources 1, 2, and 3 are "initial," and the completion of the last resource determines TTNS. Resource 4 is not "initial" as it started after the 100ms threshold.
The TTNS value is reported in the
view.network_settled_time
attribute, in nanoseconds.This is the counterpart to Android PR #2392.
How?
The metric is modeled as an object implementing the
TTNSMetricTracking
interface, which tracks key events related to TTNS (resource start, stop, and view stop) and provides a method to retrieve the metric value:The TTNS object is created within each
RUMViewScope
at the start of the view and passed toRUMResourceScope
to track resources associated with that view.This PR includes both unit and integration tests for TTNS, with integration tests located in
ViewLoadingMetricsTests
. These tests will also serve as a foundation for upcoming view loading metrics.Review checklist
make api-surface
)