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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Sep 13, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f661d45
fix: reset perf logger timer for soft navigation for SPA pages
Sep 10, 2021
a27f4e0
fix comments
Sep 10, 2021
4d065f3
fix extra comments
Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion superset-frontend/src/logger/LogUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,14 @@ export const LOG_EVENT_TYPE_USER = new Set([
]);

export const Logger = {
softNavigationStart: 0,

resetStartTime() {
this.softNavigationStart = window.performance.now();
},
Copy link
Member

@ktmud ktmud Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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);
  },


// note that this returns ms since page load, NOT ms since epoch
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
// note that this returns ms since page load, NOT ms since epoch
// note that this returns ms since last navigation, NOT ms since epoch

getTimestamp() {
return Math.round(window.performance.now());
return Math.round(window.performance.now() - this.softNavigationStart);
},
};
59 changes: 39 additions & 20 deletions superset-frontend/src/views/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,15 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { Suspense } from 'react';
import React, { Suspense, useEffect } from 'react';
import { hot } from 'react-hot-loader/root';
import { Provider as ReduxProvider } from 'react-redux';
import { BrowserRouter as Router, Switch, Route } from 'react-router-dom';
import {
BrowserRouter as Router,
Switch,
Route,
useLocation,
} from 'react-router-dom';
import { DndProvider } from 'react-dnd';
import { HTML5Backend } from 'react-dnd-html5-backend';
import { QueryParamProvider } from 'use-query-params';
Expand All @@ -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';
Copy link
Member

Choose a reason for hiding this comment

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

Please use absolute imports.

import { store } from './store';

setupApp();
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

initFeatureFlags(bootstrap.common.feature_flags);

const RootContextProviders: React.FC = ({ children }) => (
<ThemeProvider theme={theme}>
<ReduxProvider store={store}>
<DndProvider backend={HTML5Backend}>
<FlashProvider messages={common.flash_messages}>
<DynamicPluginProvider>
<QueryParamProvider
ReactRouterRoute={Route}
stringifyOptions={{ encode: false }}
>
{children}
</QueryParamProvider>
</DynamicPluginProvider>
</FlashProvider>
</DndProvider>
</ReduxProvider>
</ThemeProvider>
);
const RootContextProviders: React.FC = ({ children }) => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@graceguo-supercat graceguo-supercat Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I mean move it to the App component in L86.

Copy link
Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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)?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@etr2460 etr2460 Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Logger.resetStartTime();
}
lastLocation = location.pathname;
}, [location.pathname]);

return (
<ThemeProvider theme={theme}>
<ReduxProvider store={store}>
<DndProvider backend={HTML5Backend}>
<FlashProvider messages={common.flash_messages}>
<DynamicPluginProvider>
<QueryParamProvider
ReactRouterRoute={Route}
stringifyOptions={{ encode: false }}
>
{children}
</QueryParamProvider>
</DynamicPluginProvider>
</FlashProvider>
</DndProvider>
</ReduxProvider>
</ThemeProvider>
);
};

const App = () => (
<Router>
Expand Down