Skip to content
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-7110 Add the OOB mobile view loading metrics properties #229

Conversation

mariusc83
Copy link
Member

Following our internal RFC we are adding 2 new properties on the view-schema.json to be used for the out-of-the-box view loading time metrics we propose for the our mobile RUM sdks.

@mariusc83 mariusc83 self-assigned this Nov 13, 2024
@mariusc83 mariusc83 force-pushed the mconstantin/rum-7110/add-new-mobile-oob-view-loading-times-metrics branch 3 times, most recently from b83dc02 to ec7e7a6 Compare November 13, 2024 12:39
@mariusc83 mariusc83 marked this pull request as ready for review November 13, 2024 12:41
@mariusc83 mariusc83 requested review from a team as code owners November 13, 2024 12:41
ncreated
ncreated previously approved these changes Nov 13, 2024
@@ -31,6 +31,18 @@
"minimum": 0,
"readOnly": true
},
"network_settled_time": {
"type": "integer",
"description": "Duration in ns from the moment the initial network requests in current view started until they settled",
Copy link
Member

Choose a reason for hiding this comment

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

Note

IIRC from the RFC, the metric is measured from the moment the current view started, right, not from the start of the initial request?

Suggested change
"description": "Duration in ns from the moment the initial network requests in current view started until they settled",
"description": "Duration in ns from the moment the current view started until the initial network requests settled",

Copy link
Member Author

Choose a reason for hiding this comment

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

yes correct, my mistake here 👍

},
"interaction_to_next_view_time": {
"type": "integer",
"description": "Duration in ns to from the last interaction on previous view to the moment the next view is displayed",
Copy link
Member

Choose a reason for hiding this comment

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

Note

The metric relates to the current view, not the next (future) one.

Suggested change
"description": "Duration in ns to from the last interaction on previous view to the moment the next view is displayed",
"description": "Duration in ns to from the last interaction on previous view to the moment the current view was displayed",

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah...for me next is actually current in the context. But if you think current is better I can change it yes

@mariusc83 mariusc83 force-pushed the mconstantin/rum-7110/add-new-mobile-oob-view-loading-times-metrics branch from ec7e7a6 to 3d59884 Compare November 13, 2024 13:03
*/
readonly network_settled_time?: number;
/**
* Duration in ns to from the last interaction on previous view to the moment the current view was displayed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Duration in ns to from the last interaction on previous view to the moment the current view was displayed
* Duration in ns from the last interaction on previous view to the moment the current view was displayed

@mariusc83 mariusc83 force-pushed the mconstantin/rum-7110/add-new-mobile-oob-view-loading-times-metrics branch from 3d59884 to 9907d32 Compare November 15, 2024 08:04
@mariusc83 mariusc83 merged commit 3e308f2 into master Nov 15, 2024
8 checks passed
@mariusc83 mariusc83 deleted the mconstantin/rum-7110/add-new-mobile-oob-view-loading-times-metrics branch November 15, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants