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

Jump to bottom

[Observability] Add Observability Shared app #154716

Merged
merged 12 commits into from
Apr 11, 2023
Merged

[Observability] Add Observability Shared app #154716

merged 12 commits into from
Apr 11, 2023

Conversation

CoenWarmer
Copy link
Contributor

@CoenWarmer CoenWarmer commented Apr 11, 2023 •
edited
Loading

Resolves #154735

📝 Summary

This adds a Observability Shared package which houses the Observability Page template, and constants which are used by all Observability applications.

Why create this app?

The problem
The Synthetics team has created a widget for the Observability Overview page which requires that the Observability app requires the Exploratory View app.

The Exploratory View app needs to display the same navigation as the other Observability apps. The navigation is rendered through the Observability Page Template component, which is exported by the Observability app. That means the Exploratory View app requires the Observability app.

Both apps therefore require each other, which creates a cyclical dependency between these two apps. No bueno!

Solutions
There are three ways to solve this problem:

  1. Have the Synthetics team create a Overview page widget which does not use the Exploratory View app, thereby removing that dependency;
  2. Create a new app that houses components which are shared by all Observability apps. This new app should not depend on any of the Observability apps so all downstream apps can safely depend on it.
  3. Consolidate all Observability app inside one package so cyclical dependencies can't occur at all.

I've opted for solution 2.

Resulting structure
The structure of Observability apps will look like this when all apps are updated:

Untitled (3)

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

CoenWarmer and others added 8 commits April 11, 2023 13:41
@CoenWarmer CoenWarmer force-pushed the chore/153933-remove-exploratory-view-from-obs-app-new branch 2 times, most recently from ed73af7 to 0f07c09 Compare April 11, 2023 13:42
@CoenWarmer CoenWarmer force-pushed the chore/153933-remove-exploratory-view-from-obs-app-new branch from 9b58dd1 to 2411fad Compare April 11, 2023 14:07
@CoenWarmer CoenWarmer added release_note:skip Skip the PR/issue when compiling release notes v8.8.0 labels Apr 11, 2023
@CoenWarmer CoenWarmer marked this pull request as ready for review April 11, 2023 15:16
@CoenWarmer CoenWarmer requested a review from a team as a code owner April 11, 2023 15:16
@CoenWarmer CoenWarmer changed the title Add Observability Shared app [Observability] Add Observability Shared app Apr 11, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observabilityShared - 85 +85

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
observabilityShared - 30 +30

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observabilityShared - 37.3KB +37.3KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
observabilityShared - 4 +4

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observabilityShared - 6.0KB +6.0KB
Unknown metric groups

API count

id before after diff
observabilityShared - 30 +30

async chunk count

id before after diff
observabilityShared - 1 +1

ESLint disabled line counts

id before after diff
observabilityShared - 1 +1
securitySolution 433 436 +3
total +4

miscellaneous assets size

id before after diff
observabilityShared - 161.8KB +161.8KB

Total ESLint disabled count

id before after diff
observabilityShared - 1 +1
securitySolution 513 516 +3
total +4

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

mistic
mistic approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

Changes in the files under operations team code owners lgtm

@CoenWarmer CoenWarmer merged commit 26f65b3 into elastic:main Apr 11, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 11, 2023
This was referenced Apr 11, 2023
Closed
Merged
Closed
Closed
@sorenlouv sorenlouv mentioned this pull request Apr 28, 2023
Merged
sorenlouv added a commit that referenced this pull request May 1, 2023
@sorenlouv @kibanamachine
[APM] Avoid re-mounting the entire application on every url change (#…
66ab6b7 
…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
#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:
#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 <42973632+kibanamachine@users.noreply.github.com>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 1, 2023
@sorenlouv
[APM] Avoid re-mounting the entire application on every url change (e…
7a668c3 
…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 <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 66ab6b7)
kibanamachine added a commit that referenced this pull request May 1, 2023
@kibanamachine @sorenlouv
[8.8] [APM] Avoid re-mounting the entire application on every url cha…
775569b 
…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":"soren.louv@elastic.co"},"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
<42973632+kibanamachine@users.noreply.github.com>","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
<42973632+kibanamachine@users.noreply.github.com>","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
<42973632+kibanamachine@users.noreply.github.com>","sha":"66ab6b7464076126ee49c03a37f234280d8205a5"}}]}]
BACKPORT-->

Co-authored-by: Søren Louv-Jansen <soren.louv@elastic.co>
@alisonelizabeth alisonelizabeth mentioned this pull request May 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistic mistic mistic approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Observability] Add Observability Shared app to house shared components
5 participants
@CoenWarmer @apmmachine @kibana-ci @mistic @kibanamachine