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

Integrate react control group embeddable into dashboard container #190273

Merged
merged 109 commits into from
Aug 30, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 9, 2024

closes #191137, #190988, #191155

PR replaces legacy embeddable control group implementation with react control group implementation in DashboardContainer.

Test instructions

  1. Open dashboard via dashboard application or portable dashboard
  2. Mess around with controls. There should be no changes in behavior

@nreese
Copy link
Contributor Author

nreese commented Aug 12, 2024

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Aug 13, 2024

@elasticmachine merge upstream

@nreese nreese requested a review from Heenawter August 29, 2024 18:19
@nreese nreese requested a review from Heenawter August 29, 2024 19:35
Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Left one other comment, but everything else looks good to me. I think between our test coverage + my additional testing here, we should be in really good shape with this refactor 🤞 So excited to see this in - the code is so much cleaner!! And I'm really looking forward to the final cleanup when we officially get to remove the old controls 👀

@nreese nreese removed the ci:project-deploy-observability Create an Observability project label Aug 29, 2024
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Aug 29, 2024
@nreese
Copy link
Contributor Author

nreese commented Aug 29, 2024

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Aug 29, 2024

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented Aug 29, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: 3844856
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-190273-384485614fb6

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
controls 300 301 +1

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
controls 385 381 -4
embeddable 461 462 +1
total -3

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.5MB 3.5MB -75.0B
controls 379.6KB 380.5KB +964.0B
dashboard 463.3KB 463.1KB -205.0B
total +684.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 39.9KB 39.8KB -138.0B
controls 58.2KB 57.7KB -534.0B
dashboard 43.4KB 42.4KB -973.0B
embeddable 71.3KB 71.4KB +156.0B
total -1.5KB
Unknown metric groups

API count

id before after diff
controls 394 390 -4
embeddable 571 572 +1
total -3

async chunk count

id before after diff
dashboard 14 15 +1

References to deprecated APIs

id before after diff
dashboard 47 40 -7

History

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

@nreese nreese merged commit c272d97 into elastic:main Aug 30, 2024
23 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 30, 2024
nreese added a commit to nreese/kibana that referenced this pull request Sep 3, 2024
@thomasneirynck
Copy link
Contributor

Seems this PR has introduced a performance regression.

thomasneirynck added a commit that referenced this pull request Sep 4, 2024
…iner (#190273) (#191993)

#190273 introduced a performance regression. Reverting to move metrics to baseline again and create some time to identify root cause.

Co-authored-by: Thomas Neirynck <[email protected]>
nreese added a commit to nreese/kibana that referenced this pull request Sep 5, 2024
nreese added a commit that referenced this pull request Sep 10, 2024
…shboard container - reloaded (#192221)

PR replaces legacy embeddable control group implementation with react
control group implementation in DashboardContainer.

#### background
Work originally done in #190273.
#190273 was reverted by
#191993 because of dashboard
performance degradation. It was determined that degradation was because
new react embeddable controls fixed a regression where dashboard panels
are loading before control filters are created. This regression was
introduced by #187509.

The work around is that this PR keeps the currently broken behavior in
main and loads panels before control filters are ready. The thinking is
that the migration would replace like for like and not introduce any
performance changes. Then, at a later time, the regression could be
resolved.

#### reviewing
These are the same changes from
#190273 minus some work to
introduce a current regression in main. A full re-review is not needed.

---------

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Hannah Mudge <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Embeddable Rebuild] [Controls] Refactor DashboardContainer to use react embeddable control group
8 participants