[perf logging] Add timing event when browser tab is hidden #9733
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9733 +/- ##
==========================================
- Coverage 70.49% 70.47% -0.02%
==========================================
Files 402 402
Lines 12564 12577 +13
Branches 3112 3115 +3
==========================================
+ Hits 8857 8864 +7
- Misses 3593 3599 +6
Partials 114 114
Continue to review full report at Codecov.
|
| start_offset: Logger.getTimestamp(), | ||
| ts: new Date().getTime(), | ||
| }; | ||
| } else { |
There was a problem hiding this comment.
could we explicitly check if visibilityState === 'visible' here? I see some documentation talking about the prerender state which we probably want to ignore
| // from hidden to visible | ||
| const logStart = this.visibilityEventData.start_offset; | ||
| this.props.actions.logEvent(LOG_ACTIONS_HIDDEN_BROWSER_TAB, { | ||
| ...this.visibilityEventData.start_offset, |
There was a problem hiding this comment.
start_offset is a number i think, so wouldn't this fail when trying to spread a number?
I assume this should be ...this.visibilityEventData
| @@ -19,6 +19,7 @@ | |||
| // Log event names ------------------------------------------------------------ | |||
| export const LOG_ACTIONS_LOAD_CHART = 'load_chart'; | |||
| export const LOG_ACTIONS_RENDER_CHART = 'render_chart'; | |||
| export const LOG_ACTIONS_HIDDEN_BROWSER_TAB = 'hidden_browser_tab'; | |||
There was a problem hiding this comment.
almost all these event names use a verb to start them, although this event isn't really an action and instead is logging the duration of a state.
Maybe this should be LOG_DURATION_BROWSER_TAB_HIDDEN = 'duration_browser_tab_hidden'? I'm not really sure what a good name is here, maybe @john-bodley has thoughts?
There was a problem hiding this comment.
event name is hidden_browser_tab, duration is just one attribute of the event.
There was a problem hiding this comment.
I guess my point is, we're not logging an event when the browser tab is hidden, rather we're logging the point when the tab becomes visible again. And the most valuable part of this is the amount of time it spent in the hidden state
There was a problem hiding this comment.
This is a timing_event, like load_chart and render_chart.
To me start_offset is also valuable, I can see when user start to switch browser tab.
Even there is only one valuable attribute, duration, this hidden_browser_tab event still need to send other attribute like event_type and trigger_event so that this event will be grouped with correct dashboard load event.
So we need to send event not just an attribute.
There was a problem hiding this comment.
load_chart and render_chart start with verbs, unlike hidden_browser_tab. So I recommend we find a better name for this action. Perhaps hide_browser_tab? make_browser_tab_hidden?
There was a problem hiding this comment.
all your recommended names seems not ideal... i chose hide_browser_tab.
graceguo-supercat
force-pushed
the
gg-OpenDashHiddenTab
branch
2 times, most recently
from
3deae07
to
d5f87d8
Compare
| }; | ||
| } else if (document.visibilityState === 'visible') { | ||
| // from hidden to visible | ||
| const logStart = this.visibilityEventData.start_offset; |
There was a problem hiding this comment.
do we need this extra variable if it's only being used once?
There was a problem hiding this comment.
i feel readability will be better with an extra variable, when destructuring object with long attribute name.
graceguo-supercat
force-pushed
the
gg-OpenDashHiddenTab
branch
from
d5f87d8
to
cc6c185
Compare
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
When dashboard is landed in a hidden browser tab, charts are not fetched until user focus on the browser tab again. See this doc: https://developers.google.com/web/updates/2017/03/background_tabs
When airbnb measure dashboard performance, we count page load time is from navigation start till all charts are loaded and rendered in the dashboard. When user open dashboard in a hidden tab, for example, ctrl+click a dashboard link, dashboard will be open in a new tab, but charts will not be loaded until user focus on this tab, and the whole hidden mode time is counted as dashboard load time.
This PR is to add a timing_event in dashboard perf logging data. It should log the total duration time that dashboard is in the hidden tab.
TEST PLAN
Manual test.
REVIEWERS
@etr2460