Skip to content

Commit

Permalink
[APM] Avoid re-mounting the entire application on every url change (e…
Browse files Browse the repository at this point in the history
…lastic#156171)

## Problem
The `ErrorBoundary` component that encapsulates the APM application uses
the url path as the `key`, causing the application to be unmounted and
re-mounted on every page change:

```ts
<ErrorBoundary key={location.pathname}>{children}</ErrorBoundary>
```

## Solution

While applying a fix to the `ErrorBoundary` component I noticed that it
is no longer used. Errors are captured by the
`ObservabilityPageTemplate` (introduced a few weeks back in
elastic#154716) which makes our
ErrorBoundary useless.

The `ErrorBoundary` in `ObservabilityPageTemplate` does not have the
same problem our had. However, it also does not reset the ErrorBoundary
on page change (like ours did). This means that once an error is caught,
it is not possible to remove the Error Boundary ui without manually
performing a full page refresh. I've mentioned this to @CoenWarmer and
created a follow-up issue:
elastic#156172

## Before

https://user-images.githubusercontent.com/209966/235135444-d691b481-5441-449b-8555-77760877c3ba.mp4

## After

https://user-images.githubusercontent.com/209966/235135459-cc3d4318-d553-44ad-890b-c15bdeac9292.mp4

---------

Co-authored-by: kibanamachine <[email protected]>
  • Loading branch information
sorenlouv and kibanamachine authored May 1, 2023
1 parent 7588a96 commit 66ab6b7
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 66ab6b7

Please sign in to comment.