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 all commits
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
1 change: 1 addition & 0 deletions superset-frontend/src/dashboard/components/Dashboard.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.timeOriginOffset > 0,
is_edit_mode: dashboardState.editMode,
mount_duration: Logger.getTimestamp(),
is_empty: isDashboardEmpty(layout),
Expand Down
10 changes: 8 additions & 2 deletions 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 = {
// note that this returns ms since page load, NOT ms since epoch
timeOriginOffset: 0,

markTimeOrigin() {
this.timeOriginOffset = 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 last navigation, NOT ms since epoch
getTimestamp() {
return Math.round(window.performance.now());
return Math.round(window.performance.now() - this.timeOriginOffset);
},
};
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 'src/logger/LogUtils';
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 lastLocationPathname: string;
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 (lastLocationPathname && lastLocationPathname !== location.pathname) {
Logger.markTimeOrigin();
}
lastLocationPathname = 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