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

Revert "Integrate react control group embeddable into dashboard container (#190273) #191993

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Sep 3, 2024

Test performance of visualize embeddable refactor in isolation by reverting control group refactor

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Sep 4, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@thomasneirynck
Copy link
Contributor

This rollback reverts performance metrics back to baseline.

So it does seem #190273 introduced the regression. See #190273 (comment) for more info.

@Heenawter Heenawter added the release_note:skip Skip the PR/issue when compiling release notes label Sep 4, 2024
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
controls 437 436 -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 381 385 +4
embeddable 462 461 -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 +73.0B
controls 561.6KB 560.7KB -967.0B
dashboard 461.2KB 461.4KB +204.0B
total -690.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 40.0KB +138.0B
controls 57.8KB 58.3KB +534.0B
dashboard 42.4KB 43.3KB +973.0B
embeddable 71.4KB 71.3KB -156.0B
total +1.5KB
Unknown metric groups

API count

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

async chunk count

id before after diff
dashboard 14 13 -1

References to deprecated APIs

id before after diff
dashboard 40 47 +7

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

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.

Quick sanity check - seems to be an inverse of #190273 (file count is consistent, line numbers are inverses, contents of a few files are direct inverses). Looking in to the performance issues now, but in the mean time, reverting this LGTM 👍

@thomasneirynck thomasneirynck merged commit 86a63da into elastic:main Sep 4, 2024
25 checks passed
@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Sep 4, 2024
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
backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants