fix: reset perf logger timer for soft navigation for SPA pages #16668

graceguo-supercat · 2021-09-10T17:29:04Z

SUMMARY

We used some features from HTML5 Performance API to track dashboard page load performance. After we started to use SPA to generate and soft navigate between pages, "navigation" is any subsequent route change. Performance API's start point is based on onload event, so if user switch between chart list, dash list and dashboard (these pages are built from SPA structure), the old measurement for dashboard page load time is not accurate anymore.

This PR uses location to detect soft navigation and reset logging time start point.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE SPA

AFTER FIX

TESTING INSTRUCTIONS

CI and manual test

ADDITIONAL INFORMATION

cc @etr2460 @ktmud

codecov · 2021-09-10T17:54:15Z

Codecov Report

Merging #16668 (4d065f3) into master (4dc859f) will decrease coverage by 0.01%.
The diff coverage is 7.14%.

@@            Coverage Diff             @@
##           master   #16668      +/-   ##
==========================================
- Coverage   76.90%   76.89%   -0.02%     
==========================================
  Files        1005     1005              
  Lines       54007    54016       +9     
  Branches     7337     7338       +1     
==========================================
  Hits        41536    41536              
- Misses      12231    12240       +9     
  Partials      240      240

Flag	Coverage Δ
javascript	`71.25% <7.14%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...et-frontend/src/dashboard/components/Dashboard.jsx	`78.84% <ø> (ø)`
superset-frontend/src/views/App.tsx	`0.00% <0.00%> (ø)`
superset-frontend/src/logger/LogUtils.ts	`84.00% <33.33%> (-7.31%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc859f...4d065f3. Read the comment docs.

etr2460

a few comments

etr2460 · 2021-09-10T18:33:37Z

superset-frontend/src/logger/LogUtils.ts

+  resetStartTime() {
+    this.softNavigationStart = window.performance.now();
+  },
+
  // note that this returns ms since page load, NOT ms since epoch


Suggested change

// note that this returns ms since page load, NOT ms since epoch

// note that this returns ms since last navigation, NOT ms since epoch

etr2460 · 2021-09-10T18:35:59Z

superset-frontend/src/views/App.tsx

@@ -43,26 +49,39 @@ const bootstrap = JSON.parse(container?.getAttribute('data-bootstrap') ?? '{}');
 const user = { ...bootstrap.user };
 const menu = { ...bootstrap.common.menu_data };
 const common = { ...bootstrap.common };
+let lastLocation: string;


can we call this lastLocationPathname or lastPathname to make it more clear?

etr2460 · 2021-09-10T18:37:21Z

superset-frontend/src/views/App.tsx

+  useEffect(() => {
+    // reset performance logger timer start point to avoid soft navigation
+    // cause dashboard perf measurement problem
+    if (lastLocation && lastLocation !== location.pathname) {


Maybe a problem for future us, but will this break with slice navigation since viewing slices in explore all have the same pathnames (and instead use url params for switching which slice you're on)?

switch between slices is not a big concern for me, because in the explore view page, there is no link out to other slice(s). Like dashboard, most use cases are switching between list page and dashboard. Right now click on + button to create new slice/dashboard, are all hard navigation.

ktmud · 2021-09-10T18:51:16Z

Thanks for the fix! +1 on making perf tracking more accurate.

Is there a way in current tracking setup to distinguish between "hard landing" and "soft navigation"s? If possible, some example SQL queries on the superset.logs table would be nice.

ktmud

This looks like a valid stop-gap solution, but in the future, it might be more useful to utilize browser API like Performance.mark and Performance.measure to get even more accurate perf logging.

ktmud · 2021-09-10T18:51:54Z

superset-frontend/src/views/App.tsx

@@ -34,6 +39,7 @@ import { theme } from 'src/preamble';
 import ToastPresenter from 'src/messageToasts/containers/ToastPresenter';
 import setupApp from 'src/setup/setupApp';
 import { routes, isFrontendRoute } from 'src/views/routes';
+import { Logger } from '../logger/LogUtils';


Please use absolute imports.

ktmud · 2021-09-10T19:13:51Z

superset-frontend/src/views/App.tsx

+  useEffect(() => {
+    // reset performance logger timer start point to avoid soft navigation
+    // cause dashboard perf measurement problem
+    if (lastLocation && lastLocation !== location.pathname) {


lastLocation is a Location object so lastLocation !== location.pathname will always be true. Plus comparing with last location is not needed because useEffect already only triggers when the dependency [location.pathname] updates.

lastLocation is actually the pathname (see line 63) :P that's why i made a comment about variable naming

ktmud · 2021-09-10T19:14:48Z

superset-frontend/src/views/App.tsx

-    </ReduxProvider>
-  </ThemeProvider>
-);
+const RootContextProviders: React.FC = ({ children }) => {


Instead of putting this in RootContextProviders, it might be more appropriate to put the logic into App itself----the logger is not exactly a provider.

This logic needs to stay with global route change. Do you mean to move this logic to root dashboard / list / welcome etc, each component?

I mean move it to the App component in L86.

we chatted a little bit offline. Based on current way we use <Router> and <RootContextProviders>, keep this logic inside is probably the best solution.

graceguo-supercat · 2021-09-10T19:25:41Z

superset-frontend/src/views/App.tsx

@@ -43,26 +49,39 @@ const bootstrap = JSON.parse(container?.getAttribute('data-bootstrap') ?? '{}');
 const user = { ...bootstrap.user };
 const menu = { ...bootstrap.common.menu_data };
 const common = { ...bootstrap.common };
+let lastLocation: string;


@ktmud lastLocation is string type. I think the name is really misleading, i fixed as Erik suggested.

graceguo-supercat · 2021-09-10T19:34:18Z

superset-frontend/src/dashboard/components/Dashboard.jsx

@@ -101,6 +101,7 @@ class Dashboard extends React.PureComponent {
    const bootstrapData = appContainer?.getAttribute('data-bootstrap') || '';
    const { dashboardState, layout } = this.props;
    const eventData = {
+      is_soft_navigation: Logger.softNavigationStart > 0,


@ktmud i added flag here to track soft navigation or not.

graceguo-supercat · 2021-09-10T19:39:40Z

Thanks for the fix! +1 on making perf tracking more accurate.

Is there a way in current tracking setup to distinguish between "hard landing" and "soft navigation"s? If possible, some example SQL queries on the superset.logs table would be nice.

ktmud · 2021-09-10T19:57:03Z

superset-frontend/src/logger/LogUtils.ts

+
+  resetStartTime() {
+    this.softNavigationStart = window.performance.now();
+  },


The window.performance API refers the start time as "time origin", so how about renaming the variables like:

markTimeOrigin() { this.timeOriginOffset = window.performance.now(); } getTimestamp() { return Math.round(window.performance.now() - this.timeOriginOffset); },

…e#16668) * fix: reset perf logger timer for soft navigation for SPA pages * fix comments * fix extra comments Co-authored-by: grace_guo <grace_guo@grace-guos-MacBook-Pro.local>

fix: reset perf logger timer for soft navigation for SPA pages

f661d45

pull-request-size bot added the size/M label Sep 10, 2021

graceguo-supercat requested review from suddjian and nytai September 10, 2021 18:10

etr2460 reviewed Sep 10, 2021

View reviewed changes

ktmud reviewed Sep 10, 2021

View reviewed changes

graceguo-supercat commented Sep 10, 2021

View reviewed changes

fix comments

a27f4e0

graceguo-supercat commented Sep 10, 2021

View reviewed changes

ktmud reviewed Sep 10, 2021

View reviewed changes

fix extra comments

4d065f3

graceguo-supercat requested review from ktmud and etr2460 September 10, 2021 22:49

ktmud approved these changes Sep 13, 2021

View reviewed changes

graceguo-supercat merged commit b0b9965 into apache:master Sep 13, 2021

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: reset perf logger timer for soft navigation for SPA pages #16668

fix: reset perf logger timer for soft navigation for SPA pages #16668

graceguo-supercat commented Sep 10, 2021 •

edited

Loading

codecov bot commented Sep 10, 2021 •

edited

Loading

etr2460 left a comment

etr2460 Sep 10, 2021

etr2460 Sep 10, 2021

etr2460 Sep 10, 2021

graceguo-supercat Sep 10, 2021

ktmud commented Sep 10, 2021

ktmud left a comment

ktmud Sep 10, 2021

ktmud Sep 10, 2021

etr2460 Sep 10, 2021 •

edited

Loading

ktmud Sep 10, 2021

graceguo-supercat Sep 10, 2021 •

edited

Loading

ktmud Sep 10, 2021

graceguo-supercat Sep 10, 2021

graceguo-supercat Sep 10, 2021

graceguo-supercat Sep 10, 2021

graceguo-supercat commented Sep 10, 2021

ktmud Sep 10, 2021 •

edited

Loading

	// note that this returns ms since page load, NOT ms since epoch
	// note that this returns ms since last navigation, NOT ms since epoch

fix: reset perf logger timer for soft navigation for SPA pages #16668

fix: reset perf logger timer for soft navigation for SPA pages #16668

Conversation

graceguo-supercat commented Sep 10, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 10, 2021 • edited Loading

Codecov Report

etr2460 left a 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

ktmud commented Sep 10, 2021

ktmud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Sep 10, 2021 • edited Loading

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

graceguo-supercat commented Sep 10, 2021

ktmud Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

graceguo-supercat commented Sep 10, 2021 •

edited

Loading

codecov bot commented Sep 10, 2021 •

edited

Loading

etr2460 Sep 10, 2021 •

edited

Loading

graceguo-supercat Sep 10, 2021 •

edited

Loading

ktmud Sep 10, 2021 •

edited

Loading