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.

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

Use Observability Page Template from Observability Shared in APM and Profiling #154776

Conversation

CoenWarmer
Copy link
Contributor

@CoenWarmer CoenWarmer commented Apr 11, 2023

Resolves #154775

📝 Summary

As part of #154716 a Observability Shared app was created, which exports the Observability Page Template.

This PR updates the APM and Profiling apps to use the Observability Page Template from Observability Shared.

More details about the change in the PR linked above. Summarized, this change comes down to:

Untitled (3)

✅ Checklist

  • All links and navigation should behave exactly as before.

@CoenWarmer CoenWarmer requested review from a team as code owners April 11, 2023 20:18
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Apr 11, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

@CoenWarmer CoenWarmer added the release_note:skip Skip the PR/issue when compiling release notes label Apr 11, 2023
@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 CoenWarmer force-pushed the chore/use-observability-page-template-from-observability-shared-in-apm-and-profiling branch from 006257d to eea33ef Compare April 11, 2023 20:53
@CoenWarmer CoenWarmer requested a review from a team as a code owner April 11, 2023 21:12
@CoenWarmer
Copy link
Contributor Author

@elasticmachine merge upstream

@CoenWarmer
Copy link
Contributor Author

@elasticmachine merge upstream

@CoenWarmer
Copy link
Contributor Author

@elasticmachine merge upstream

@CoenWarmer
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

Profiling changes LGTM. But I added a question that needs clarification.

Comment on lines 15 to +16
"observability",
"observabilityShared",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between observability and observability_shared? By reading each README file looks like they exist for the same purpose.

Copy link
Contributor Author

@CoenWarmer CoenWarmer Apr 12, 2023

Choose a reason for hiding this comment

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

@cauemarcondes

There's two primary reasons for this change, one conceptual and one practical.

  1. When the Observability app started, it was just the Overview page. It made sense to use it as a sharing place for all downstream Observability apps. Over time a lot more routes have been added: we are now at 9 routes and counting. There is so much dedicated logic inside the app that it starts to make less sense to keep it as a shared repository dictating how the other apps should work. I believe it should more be like a downstream app that consumes shared resources like all the other Observability apps do.
  2. The Synthetics team has placed a component in the Observability app which requires the Exploratory View app. But the Exploratory View app requires the Observability app as it shares the same navigation. This creates a circular dependency. This change enables that both the Observability app and Exploratory View can share the same page template, and that the Observability app can require the Exploratory View app so it can render the Synthetics widget.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the Observability plugin also depend on the Observability_shared plugin? Or can it? Is there a plan to "clean up" the observability plugin by moving stuff that is actually shared to the new plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cauemarcondes

Does the Observability plugin also depend on the Observability_shared plugin?

Yes, or at least, it will in an upcoming PR.

Is there a plan to "clean up" the observability plugin by moving stuff that is actually shared to the new plugin?

Yes, I intend to kick out all shared stuff to Observability_shared.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
apm 3.4MB 3.4MB +54.0B

Page load bundle

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

id before after diff
apm 32.0KB 32.0KB +6.0B
profiling 14.3KB 14.3KB +6.0B
total +12.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 432 435 +3

Total ESLint disabled count

id before after diff
securitySolution 512 515 +3

History

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

@yngrdyn
Copy link
Contributor

yngrdyn commented Apr 17, 2023

/oblt-deploy

Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

APM Changes LGTM 🌟

@CoenWarmer CoenWarmer merged commit c758633 into elastic:main Apr 18, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Observability Page Template from Observability Shared in APM and Profiling
8 participants