Improve Chat Switch timing in Grafana so it waits for all messages to layout #2684

marcaaron · 2021-05-04T19:15:06Z

Details

While A/B testing changes to Onyx cache stuff we discovered that this test does not actually measure the time it takes to tap on a chat and see the entire contents of the chat rendered. What we were testing is the time it takes the ReportActionsView to mount, but it might have no chat history available to it yet.

This new test will time how long it takes for all the items to render by debouncing a call passed to the onLayout of the view enclosing the chat content so it will record the "end" event once no one has called it after 1 second. We'll also then need to offset the timing by 1 second so that we get the true result.

This will give us a better sense of when the report is "ready to view".

Note: This test has one downside at the moment which is that @kidroca has pointed out that VirtualizedList may be configured incorrectly and rendering many more items than necessary see issue here. Still I think it's valuable to do this test now so we can understand when a report is actually done doing it's "work" to render the UI.

Fixed Issues

Fixes #2683

Tests

Switch to a chat
Wait for the chat messages to load
Verify the timing log eventually shows with the time it took for all chat messages to render

QA Steps

No QA

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Julesssss · 2021-05-05T11:11:12Z

src/pages/home/report/ReportActionItem.js

@@ -122,7 +125,7 @@ class ReportActionItem extends Component {
                            {this.props.shouldDisplayNewIndicator && (
                                <UnreadActionIndicator />
                            )}
-                            <View style={getReportActionItemStyle(hovered)}>
+                            <View style={getReportActionItemStyle(hovered)} onLayout={this.props.onLayout}>


Oh nice, didn't know that View has an onLayout prop.

https://reactnative.dev/docs/view#onlayout

The docs do say

This event is fired immediately once the layout has been calculated, but the new layout may not yet be reflected on the screen at the time the event is received, especially if a layout animation is in progress.

I think this is maybe as close as we can get to treating an individual chat message as "rendered". There might be a better indicator to use.

Hey I thought that as well, I don't mean to get you sidetracked but here's what I've found looking for an alternative:

viewabilityConfigCallbackPairs

List of ViewabilityConfig/onViewableItemsChanged pairs. A specific onViewableItemsChanged will be called when its corresponding ViewabilityConfig's conditions are met. See ViewabilityHelper.js for flow type and further documentation.

And in ViewabilityHelper.js

Minimum amount of time (in milliseconds) that an item must be physically viewable before the
viewability callback will be fired.

I've never used this but it seems you can configure this and wait for the callback to get called for all the items or just the first 5-10 (Thought what about when there are less than 10...)

Anyway I think onLayout is close enough maybe 1-2 frames before the item is rendered so that like 16-32ms which is probably too little to care about

Thanks, that's interesting! The docs are kind of sparse on that stuff so I'm not too sure if I'll go down this road right now. I think it will maybe be slightly more complicated to use this then to do the naive idea I had.

Mostly because onViewableItemsChanged is called not just when the items first appear, but when things scroll etc. And I don't want to think about it 😄

Julesssss

LGTM. Will keep in mind for future render timing.

Julesssss · 2021-05-05T11:13:44Z

All yours @MonilBhavsar

Julesssss · 2021-05-05T11:19:57Z

src/CONST.js

@@ -64,6 +64,7 @@ const CONST = {
        HOMEPAGE_REPORTS_LOADED: 'homepage_reports_loaded',
        SWITCH_REPORT: 'switch_report',
        COLD: 'cold',
+        REPORT_ACTION_ITEM_LAYOUT_DEBOUNCE_TIME: 1000,


NAB, but is it true that a slow child component taking longer than the denounce time to render would trigger a false measurement?

Yes, I think if we had a child that takes more than a second to render we'd definitely get a funky measurement.

I think it's pretty unlikely for that to happen.

I came across another issue here though that I'm going to look into addressing now.

If we switch chats before thing are done rendering it's possible to get a negative value recorded so I'll look into how to prevent this before proceeding.

Ok figured that one out and we just need to cancel the debounced method in componentWillUnmount() so it does not incorrectly set the timer end when switching to the next chat.

So, do you think it's OK to have this... or should we perhaps increase the time?

a slow child component taking longer than the denounce time to render would trigger a false measurement

As unlikely as this is, I think it would be better to set a slightly higher debounce time -- just in case. But it's NAB. I'll leave it up to you 👍

Gonna bump it to 1500

MonilBhavsar · 2021-05-05T15:06:38Z

@Julesssss, the code looks good to me.
You can merge after your conversation

Julesssss · 2021-05-05T16:07:43Z

@Julesssss, the code looks good to me.
You can merge after your conversation

Thanks @MonilBhavsar. Don't forget to approve the PR if you are happy with it, else the review won't count towards your total and you'll be auto-assigned more PRs for review :)

Also just to let you know, NAB == 'Not A Blocker'. You might see people adding NAB if a particular comment or change request is not essential and should not block the merge.

MonilBhavsar · 2021-05-05T16:34:38Z

Yes sure, Approving it, From today only I got PR Reviews and I got a lot
and NAB makes sense now

Julesssss · 2021-05-05T18:36:53Z

I'd like to run the test once more, but unfortunately ran out of time this afternoon. Will make this my first task tomorrow. Once that's one I'll merge.

Julesssss · 2021-05-06T11:17:22Z

I'm leaving the denounce time increase up to @marcaaron, feel free to merge.

OSBotify · 2021-05-10T09:22:36Z

🚀 Deployed to staging in version: 1.0.41-2🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-05-13T23:05:15Z

🚀 Deployed to production in version: 1.0.44-0🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron added 2 commits May 4, 2021 07:43

move to report actions view

99a972b

Use the debounce time in CONST

6ee24bd

marcaaron requested a review from a team as a code owner May 4, 2021 19:15

marcaaron self-assigned this May 4, 2021

improve comment

ba525f0

MelvinBot requested review from MonilBhavsar and removed request for a team May 4, 2021 19:15

marcaaron requested a review from Julesssss May 4, 2021 19:17

Julesssss reviewed May 5, 2021

View reviewed changes

Julesssss previously approved these changes May 5, 2021

View reviewed changes

Julesssss reviewed May 5, 2021

View reviewed changes

MonilBhavsar previously approved these changes May 5, 2021

View reviewed changes

marcaaron added 2 commits May 5, 2021 10:27

Merge remote-tracking branch 'origin' into marcaaron-chatSwitchTest

2f66572

Cancel the debounce to avoid firing it after switching

778400c

marcaaron dismissed stale reviews from MonilBhavsar and Julesssss via 778400c May 5, 2021 20:54

Julesssss previously approved these changes May 6, 2021

View reviewed changes

marcaaron added 2 commits May 7, 2021 09:52

Fix conflict

4277ea6

increase debounce time slightl

72d2a94

marcaaron dismissed Julesssss’s stale review via 72d2a94 May 7, 2021 19:52

marcaaron requested review from Julesssss and MonilBhavsar May 7, 2021 19:53

MonilBhavsar approved these changes May 10, 2021

View reviewed changes

Julesssss approved these changes May 10, 2021

View reviewed changes

Julesssss merged commit 04d9caa into main May 10, 2021

Julesssss deleted the marcaaron-chatSwitchTest branch May 10, 2021 08:56

roryabraham mentioned this pull request May 11, 2021

Deploy Checklist: Expensify.cash 2021-05-08 #2751

Closed

36 tasks

Julesssss mentioned this pull request Jul 13, 2021

[Performance] Grafana logs don't seem to make sense anymore #3889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Chat Switch timing in Grafana so it waits for all messages to layout #2684

Improve Chat Switch timing in Grafana so it waits for all messages to layout #2684

marcaaron commented May 4, 2021 •

edited

Loading

Julesssss May 5, 2021

marcaaron May 5, 2021

kidroca May 5, 2021

kidroca May 5, 2021

marcaaron May 5, 2021

marcaaron May 5, 2021

Julesssss left a comment

Julesssss commented May 5, 2021

Julesssss May 5, 2021

marcaaron May 5, 2021

marcaaron May 5, 2021

Julesssss May 6, 2021

marcaaron May 7, 2021

MonilBhavsar commented May 5, 2021

Julesssss commented May 5, 2021

MonilBhavsar commented May 5, 2021

Julesssss commented May 5, 2021 •

edited

Loading

Julesssss commented May 6, 2021

OSBotify commented May 10, 2021

OSBotify commented May 13, 2021

Improve Chat Switch timing in Grafana so it waits for all messages to layout #2684

Improve Chat Switch timing in Grafana so it waits for all messages to layout #2684

Conversation

marcaaron commented May 4, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

Julesssss commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonilBhavsar commented May 5, 2021

Julesssss commented May 5, 2021

MonilBhavsar commented May 5, 2021

Julesssss commented May 5, 2021 • edited Loading

Julesssss commented May 6, 2021

OSBotify commented May 10, 2021

OSBotify commented May 13, 2021

marcaaron commented May 4, 2021 •

edited

Loading

Julesssss commented May 5, 2021 •

edited

Loading