Skip to content

Commit

Permalink
[8.8] [APM] Avoid re-mounting the entire application on every url cha…
Browse files Browse the repository at this point in the history
…nge (#156171) (#156309)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[APM] Avoid re-mounting the entire application on every url change
(#156171)](#156171)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Søren
Louv-Jansen","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-01T21:46:17Z","message":"[APM]
Avoid re-mounting the entire application on every url change
(#156171)\n\n## Problem\r\nThe `ErrorBoundary` component that
encapsulates the APM application uses\r\nthe url path as the `key`,
causing the application to be unmounted and\r\nre-mounted on every page
change:\r\n\r\n```ts\r\n<ErrorBoundary
key={location.pathname}>{children}</ErrorBoundary>\r\n```\r\n\r\n##
Solution\r\n\r\nWhile applying a fix to the `ErrorBoundary` component I
noticed that it\r\nis no longer used. Errors are captured by
the\r\n`ObservabilityPageTemplate` (introduced a few weeks back
in\r\nhttps://github.com//pull/154716) which makes
our\r\nErrorBoundary useless.\r\n\r\nThe `ErrorBoundary` in
`ObservabilityPageTemplate` does not have the\r\nsame problem our had.
However, it also does not reset the ErrorBoundary\r\non page change
(like ours did). This means that once an error is caught,\r\nit is not
possible to remove the Error Boundary ui without manually\r\nperforming
a full page refresh. I've mentioned this to @CoenWarmer and\r\ncreated a
follow-up
issue:\r\nhttps://github.com//issues/156172\r\n\r\n##
Before\r\n\r\nhttps://user-images.githubusercontent.com/209966/235135444-d691b481-5441-449b-8555-77760877c3ba.mp4\r\n\r\n##
After\r\n\r\nhttps://user-images.githubusercontent.com/209966/235135459-cc3d4318-d553-44ad-890b-c15bdeac9292.mp4\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"66ab6b7464076126ee49c03a37f234280d8205a5","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:APM","release_note:skip","v8.8.0","v8.9.0"],"number":156171,"url":"https://github.com/elastic/kibana/pull/156171","mergeCommit":{"message":"[APM]
Avoid re-mounting the entire application on every url change
(#156171)\n\n## Problem\r\nThe `ErrorBoundary` component that
encapsulates the APM application uses\r\nthe url path as the `key`,
causing the application to be unmounted and\r\nre-mounted on every page
change:\r\n\r\n```ts\r\n<ErrorBoundary
key={location.pathname}>{children}</ErrorBoundary>\r\n```\r\n\r\n##
Solution\r\n\r\nWhile applying a fix to the `ErrorBoundary` component I
noticed that it\r\nis no longer used. Errors are captured by
the\r\n`ObservabilityPageTemplate` (introduced a few weeks back
in\r\nhttps://github.com//pull/154716) which makes
our\r\nErrorBoundary useless.\r\n\r\nThe `ErrorBoundary` in
`ObservabilityPageTemplate` does not have the\r\nsame problem our had.
However, it also does not reset the ErrorBoundary\r\non page change
(like ours did). This means that once an error is caught,\r\nit is not
possible to remove the Error Boundary ui without manually\r\nperforming
a full page refresh. I've mentioned this to @CoenWarmer and\r\ncreated a
follow-up
issue:\r\nhttps://github.com//issues/156172\r\n\r\n##
Before\r\n\r\nhttps://user-images.githubusercontent.com/209966/235135444-d691b481-5441-449b-8555-77760877c3ba.mp4\r\n\r\n##
After\r\n\r\nhttps://user-images.githubusercontent.com/209966/235135459-cc3d4318-d553-44ad-890b-c15bdeac9292.mp4\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"66ab6b7464076126ee49c03a37f234280d8205a5"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156171","number":156171,"mergeCommit":{"message":"[APM]
Avoid re-mounting the entire application on every url change
(#156171)\n\n## Problem\r\nThe `ErrorBoundary` component that
encapsulates the APM application uses\r\nthe url path as the `key`,
causing the application to be unmounted and\r\nre-mounted on every page
change:\r\n\r\n```ts\r\n<ErrorBoundary
key={location.pathname}>{children}</ErrorBoundary>\r\n```\r\n\r\n##
Solution\r\n\r\nWhile applying a fix to the `ErrorBoundary` component I
noticed that it\r\nis no longer used. Errors are captured by
the\r\n`ObservabilityPageTemplate` (introduced a few weeks back
in\r\nhttps://github.com//pull/154716) which makes
our\r\nErrorBoundary useless.\r\n\r\nThe `ErrorBoundary` in
`ObservabilityPageTemplate` does not have the\r\nsame problem our had.
However, it also does not reset the ErrorBoundary\r\non page change
(like ours did). This means that once an error is caught,\r\nit is not
possible to remove the Error Boundary ui without manually\r\nperforming
a full page refresh. I've mentioned this to @CoenWarmer and\r\ncreated a
follow-up
issue:\r\nhttps://github.com//issues/156172\r\n\r\n##
Before\r\n\r\nhttps://user-images.githubusercontent.com/209966/235135444-d691b481-5441-449b-8555-77760877c3ba.mp4\r\n\r\n##
After\r\n\r\nhttps://user-images.githubusercontent.com/209966/235135459-cc3d4318-d553-44ad-890b-c15bdeac9292.mp4\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"66ab6b7464076126ee49c03a37f234280d8205a5"}}]}]
BACKPORT-->

Co-authored-by: Søren Louv-Jansen <[email protected]>
  • Loading branch information
kibanamachine and sorenlouv authored May 1, 2023
1 parent 8d5b222 commit 775569b
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 22 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import url from 'url';
import { synthtrace } from '../../../synthtrace';
import { opbeans } from '../../fixtures/synthtrace/opbeans';

const start = '2021-10-10T00:00:00.000Z';
const end = '2021-10-10T00:15:00.000Z';

const serviceOverview = url.format({
pathname: '/app/apm/services/opbeans-java/overview',
query: {
comparisonEnabled: 'true',
environment: 'ENVIRONMENT_ALL',
rangeFrom: start,
rangeTo: end,
offset: '1d',
},
});

describe('When navigating between pages', () => {
before(() => {
synthtrace.index(
opbeans({
from: new Date(start).getTime(),
to: new Date(end).getTime(),
})
);
});

after(() => {
synthtrace.clean();
});

beforeEach(() => {
cy.loginAsViewerUser();
});

it('should only load certain resources once', () => {
cy.intercept('**/internal/apm/has_data').as('hasDataRequest');
cy.intercept('**/internal/apm/services/opbeans-java/metadata/icons**').as(
'serviceIconsRequest'
);
cy.intercept('**/apm/fleet/has_apm_policies').as('apmPoliciesRequest');

// Overview page
cy.visitKibana(serviceOverview);
cy.get('.euiTab-isSelected').should('have.text', 'Overview');

// it should load resources once
cy.get('@hasDataRequest.all').should('have.length', 1);
cy.get('@serviceIconsRequest.all').should('have.length', 1);
cy.get('@apmPoliciesRequest.all').should('have.length', 1);

// Navigate to errors page
cy.contains('.euiTab', 'Errors').click();
cy.get('.euiTab-isSelected').should('have.text', 'Errors');

// page have loaded correctly
cy.get('.euiLoadingChart').should('not.exist');
cy.get('[data-test-subj="errorDistribution"]').should('exist');

// it should not load resources again
cy.get('@hasDataRequest.all').should('have.length', 1);
cy.get('@serviceIconsRequest.all').should('have.length', 1);
cy.get('@apmPoliciesRequest.all').should('have.length', 1);
});
});
17 changes: 0 additions & 17 deletions x-pack/plugins/apm/public/application/routes/index.tsx

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,27 @@ import { ApmPluginStartDeps } from '../../plugin';

export function ApmErrorBoundary({ children }: { children?: React.ReactNode }) {
const location = useLocation();
return <ErrorBoundary key={location.pathname}>{children}</ErrorBoundary>;
return <ErrorBoundary pathname={location.pathname}>{children}</ErrorBoundary>;
}

class ErrorBoundary extends React.Component<
{ children?: React.ReactNode },
{ children?: React.ReactNode; pathname: string },
{ error?: Error },
{}
> {
public state: { error?: Error } = {
error: undefined,
};

componentDidUpdate(prevProps: { pathname: string }) {
if (
this.props.pathname !== prevProps.pathname &&
this.state.error !== undefined
) {
this.setState({ error: undefined });
}
}

static getDerivedStateFromError(error: Error) {
return { error };
}
Expand Down Expand Up @@ -66,7 +75,6 @@ function ErrorWithTemplate({ error }: { error: Error }) {
);
}

function DummyComponent({ error }: { error: Error }) {
function DummyComponent({ error }: { error: Error }): any {
throw error;
return <div />;
}
2 changes: 1 addition & 1 deletion x-pack/plugins/apm/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@
"@kbn/babel-register",
"@kbn/core-saved-objects-migration-server-internal",
"@kbn/core-elasticsearch-server",
"@kbn/shared-ux-prompt-not-found",
"@kbn/core-saved-objects-api-server",
"@kbn/safer-lodash-set",
"@kbn/shared-ux-router",
Expand All @@ -85,6 +84,7 @@
"@kbn/logging-mocks",
"@kbn/chart-icons",
"@kbn/observability-shared-plugin",
"@kbn/shared-ux-prompt-not-found",
],
"exclude": [
"target/**/*",
Expand Down

0 comments on commit 775569b

Please sign in to comment.