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

[maps] fix EMS vector file manifest is loaded in Discover #189550

Merged
merged 4 commits into from
Aug 5, 2024
Merged

[maps] fix EMS vector file manifest is loaded in Discover #189550

merged 4 commits into from
Aug 5, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jul 30, 2024 •
edited
Loading

Closes #157429

Choropleth lens chart registerVisualization is called during Discover page load. This PR removes loading of EMS files from registerVisualization to avoid loading EMS files on Discover page load.

There is a trade off since EMS files are needed in synchronous functions such as getSuggestions. Before EMS files are loaded, getSuggestions will return no suggestions.

Test instructions

  1. Load discover page with network tab open. Verify no requests are made to vector.maps.elastic.co
  2. Create a new lens visualization. Drag geo.src field from sample web logs into the center. Verify region map suggestion is displayed. Click it. Verify you can edit region key field.
    Screenshot 2024-07-30 at 12 04 58 PM

@nreese
Copy link
Contributor Author

nreese commented Jul 30, 2024

/ci

@nreese nreese marked this pull request as ready for review July 30, 2024 20:19
@nreese nreese requested a review from a team as a code owner July 30, 2024 20:19
@nreese nreese added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:skip Skip the PR/issue when compiling release notes Feature:Maps v8.16.0 labels Jul 30, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@nreese
Copy link
Contributor Author

nreese commented Aug 1, 2024

@elasticmachine merge upstream

thomasneirynck
thomasneirynck approved these changes Aug 1, 2024
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

code review. this is a nice cleanup with low impact on users imho, thx for adding.

@nreese nreese changed the title fix vector file manifest is loaded in Discover [maps] fix EMS vector file manifest is loaded in Discover Aug 5, 2024
@nreese
Copy link
Contributor Author

nreese commented Aug 5, 2024

@elasticmachine merge upstream

nickpeihl
nickpeihl approved these changes Aug 5, 2024
View reviewed changes
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for fixing this.

code review and tested field statistics in Discover and getSuggestions in Lens with and without access to EMS.

@nreese
Copy link
Contributor Author

nreese commented Aug 5, 2024

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
maps 1216 1217 1

Async chunks

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

id before after diff
maps 3.0MB 3.0MB 2.8KB

Page load bundle

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

id before after diff
maps 54.7KB 54.3KB -387.0B
Unknown metric groups

async chunk count

id before after diff
maps 32 34 2

ESLint disabled line counts

id before after diff
maps 50 51 1

Total ESLint disabled count

id before after diff
maps 77 78 1

History

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

@nreese nreese merged commit 5267f4a into elastic:main Aug 5, 2024
19 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 5, 2024
nreese added a commit to nreese/kibana that referenced this pull request Aug 6, 2024
@nreese
Revert "[maps] fix EMS vector file manifest is loaded in Discover (el…
469cf73 
…astic#189550)"

This reverts commit 5267f4a.
@nreese nreese mentioned this pull request Aug 6, 2024
Merged
nreese added a commit that referenced this pull request Aug 6, 2024
@nreese @elasticmachine
Revert "[maps] fix EMS vector file manifest is loaded in Discover" (#…
616f567 
…189984)

Around the time #189550 merged,
there was a drop in dashboard loading times without maps. Reverting PR
to see if there is a corresponding time increase to test if this fix
resulted in the performance gain.

Revert is temeratory and fixes will be re-instated once performance
tests are complete.

Co-authored-by: Elastic Machine <[email protected]>
@nreese nreese mentioned this pull request Aug 7, 2024
Merged
nreese added a commit that referenced this pull request Aug 7, 2024
@nreese
[maps] Revert "Revert of fix EMS vector file manifest is loaded in Di…
e2fdced 
…scover (#190073)

Re-instate #189550. PR makes one
small clean-up in that getSuggestionsLazy promise will not reject when
modules fail to load.

#189984 confirmed that [the fix
did](#189550) had a large impact
on performance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasneirynck thomasneirynck thomasneirynck approved these changes

@nickpeihl nickpeihl nickpeihl approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Maps release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Discover][Maps] vector file manifest is loaded in Discover
6 participants
@nreese @elasticmachine @kibana-ci @nickpeihl @thomasneirynck @kibanamachine