You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/09/10 17:29:13 UTC
[GitHub] [superset] graceguo-supercat opened a new pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
graceguo-supercat opened a new pull request #16668:
URL: https://github.com/apache/superset/pull/16668
### 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
### TESTING INSTRUCTIONS
CI and manual test
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706436109
##########
File path: superset-frontend/src/logger/LogUtils.ts
##########
@@ -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
+ softNavigationStart: 0,
+
+ resetStartTime() {
+ this.softNavigationStart = window.performance.now();
+ },
Review comment:
The `window.performance` API refers the start time as ["time origin"](https://developer.mozilla.org/en-US/docs/Web/API/DOMHighResTimeStamp#the_time_origin), so how about renaming the variables like:
```ts
markTimeOrigin() {
this.timeOriginOffset = window.performance.now();
}
getTimestamp() {
return Math.round(window.performance.now() - this.timeOriginOffset);
},
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16668:
URL: https://github.com/apache/superset/pull/16668#issuecomment-917097794
# [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16668](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a27f4e0) into [master](https://codecov.io/gh/apache/superset/commit/4dc859f89e5668ed3f94cd1ef0532a301a3ab85a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4dc859f) will **decrease** coverage by `0.01%`.
> The diff coverage is `7.14%`.
> :exclamation: Current head a27f4e0 differs from pull request most recent head 4d065f3. Consider uploading reports for the commit 4d065f3 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `78.84% <ø> (ø)` | |
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/logger/LogUtils.ts](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2xvZ2dlci9Mb2dVdGlscy50cw==) | `84.00% <33.33%> (-7.31%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4dc859f...4d065f3](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706412999
##########
File path: 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;
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 }) => {
+ const location = useLocation();
+ useEffect(() => {
+ // reset performance logger timer start point to avoid soft navigation
+ // cause dashboard perf measurement problem
+ if (lastLocation && lastLocation !== location.pathname) {
Review comment:
`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.
##########
File path: 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';
Review comment:
Please use absolute imports.
##########
File path: 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;
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 }) => {
Review comment:
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] graceguo-supercat commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706413139
##########
File path: 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;
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 }) => {
+ const location = useLocation();
+ useEffect(() => {
+ // reset performance logger timer start point to avoid soft navigation
+ // cause dashboard perf measurement problem
+ if (lastLocation && lastLocation !== location.pathname) {
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] graceguo-supercat commented on pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #16668:
URL: https://github.com/apache/superset/pull/16668#issuecomment-917163262
> 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.
<img width="598" alt="Screen Shot 2021-09-10 at 12 23 19 PM" src="https://user-images.githubusercontent.com/27990562/132908294-6173e37f-a5bc-4572-bfc3-68519030041c.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16668:
URL: https://github.com/apache/superset/pull/16668#issuecomment-917097794
# [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16668](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4d065f3) into [master](https://codecov.io/gh/apache/superset/commit/4dc859f89e5668ed3f94cd1ef0532a301a3ab85a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4dc859f) will **decrease** coverage by `0.01%`.
> The diff coverage is `7.14%`.
[](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `78.84% <ø> (ø)` | |
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/logger/LogUtils.ts](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2xvZ2dlci9Mb2dVdGlscy50cw==) | `84.00% <33.33%> (-7.31%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4dc859f...4d065f3](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706418059
##########
File path: 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;
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 }) => {
+ const location = useLocation();
+ useEffect(() => {
+ // reset performance logger timer start point to avoid soft navigation
+ // cause dashboard perf measurement problem
+ if (lastLocation && lastLocation !== location.pathname) {
Review comment:
lastLocation is actually the pathname (see line 63) :P that's why i made a comment about variable naming
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] graceguo-supercat commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706419330
##########
File path: 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;
Review comment:
@ktmud lastLocation is string type. I think the name is really misleading, i fixed as Erik suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] graceguo-supercat commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706422340
##########
File path: 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;
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 }) => {
Review comment:
This logic needs to stay with _global_ route change. Do you mean to move this logic to root dashboard / list / welcome etc, each component?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706436109
##########
File path: superset-frontend/src/logger/LogUtils.ts
##########
@@ -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
+ softNavigationStart: 0,
+
+ resetStartTime() {
+ this.softNavigationStart = window.performance.now();
+ },
Review comment:
The `window.performance` refers the start time as ["time origin"](https://developer.mozilla.org/en-US/docs/Web/API/DOMHighResTimeStamp#the_time_origin), so how about renaming the variables like:
```ts
markTimeOrigin() {
this.timeOriginOffset = window.performance.now();
}
getTimestamp() {
return Math.round(window.performance.now() - this.timeOriginOffset);
},
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #16668:
URL: https://github.com/apache/superset/pull/16668#issuecomment-917097794
# [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16668](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f661d45) into [master](https://codecov.io/gh/apache/superset/commit/4dc859f89e5668ed3f94cd1ef0532a301a3ab85a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4dc859f) will **decrease** coverage by `0.01%`.
> The diff coverage is `7.14%`.
[](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/logger/LogUtils.ts](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2xvZ2dlci9Mb2dVdGlscy50cw==) | `84.00% <33.33%> (-7.31%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4dc859f...f661d45](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] graceguo-supercat merged pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #16668:
URL: https://github.com/apache/superset/pull/16668
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16668:
URL: https://github.com/apache/superset/pull/16668#issuecomment-917097794
# [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16668](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a27f4e0) into [master](https://codecov.io/gh/apache/superset/commit/4dc859f89e5668ed3f94cd1ef0532a301a3ab85a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4dc859f) will **decrease** coverage by `0.01%`.
> The diff coverage is `7.14%`.
[](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `78.84% <ø> (ø)` | |
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/logger/LogUtils.ts](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2xvZ2dlci9Mb2dVdGlscy50cw==) | `84.00% <33.33%> (-7.31%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4dc859f...a27f4e0](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] graceguo-supercat commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706422340
##########
File path: 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;
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 }) => {
Review comment:
This logic needs to stay with _global_ route change. Do you mean to move this logic to root dashboard / list component?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706418059
##########
File path: 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;
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 }) => {
+ const location = useLocation();
+ useEffect(() => {
+ // reset performance logger timer start point to avoid soft navigation
+ // cause dashboard perf measurement problem
+ if (lastLocation && lastLocation !== location.pathname) {
Review comment:
lastLocation is actually the pathname (see line 63) :P that's why i commented about variable naming in my comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #16668:
URL: https://github.com/apache/superset/pull/16668#issuecomment-917133880
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706433886
##########
File path: 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;
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 }) => {
Review comment:
I mean move it to the `App` component in L86.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706391700
##########
File path: 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;
Review comment:
can we call this `lastLocationPathname` or `lastPathname` to make it more clear?
##########
File path: 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;
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 }) => {
+ const location = useLocation();
+ useEffect(() => {
+ // reset performance logger timer start point to avoid soft navigation
+ // cause dashboard perf measurement problem
+ if (lastLocation && lastLocation !== location.pathname) {
Review comment:
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)?
##########
File path: superset-frontend/src/logger/LogUtils.ts
##########
@@ -59,8 +59,14 @@ export const LOG_EVENT_TYPE_USER = new Set([
]);
export const Logger = {
+ softNavigationStart: 0,
+
+ resetStartTime() {
+ this.softNavigationStart = window.performance.now();
+ },
+
// note that this returns ms since page load, NOT ms since epoch
Review comment:
```suggestion
// note that this returns ms since last navigation, NOT ms since epoch
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706418059
##########
File path: 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;
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 }) => {
+ const location = useLocation();
+ useEffect(() => {
+ // reset performance logger timer start point to avoid soft navigation
+ // cause dashboard perf measurement problem
+ if (lastLocation && lastLocation !== location.pathname) {
Review comment:
lastLocation is actually the pathname :P that's why i commented about variable naming in my comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16668:
URL: https://github.com/apache/superset/pull/16668#issuecomment-917097794
# [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16668](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f661d45) into [master](https://codecov.io/gh/apache/superset/commit/4dc859f89e5668ed3f94cd1ef0532a301a3ab85a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4dc859f) will **decrease** coverage by `0.01%`.
> The diff coverage is `7.14%`.
> :exclamation: Current head f661d45 differs from pull request most recent head a27f4e0. Consider uploading reports for the commit a27f4e0 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/logger/LogUtils.ts](https://codecov.io/gh/apache/superset/pull/16668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2xvZ2dlci9Mb2dVdGlscy50cw==) | `84.00% <33.33%> (-7.31%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4dc859f...a27f4e0](https://codecov.io/gh/apache/superset/pull/16668?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] graceguo-supercat commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706507263
##########
File path: 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;
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 }) => {
Review comment:
we chatted a little bit offline. Based on current way we use `<Router>` and `<RootContextProviders>`, keep this logic inside is probably the best solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] graceguo-supercat commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706424235
##########
File path: 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,
Review comment:
@ktmud i added flag here to track soft navigation or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org