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

Add @kbn/analytics-client to the shared libs to test any bundle size improvements #130027

Closed
Tracked by #121992
afharo opened this issue Apr 12, 2022 · 9 comments
Closed
Tracked by #121992
Labels
Feature:Telemetry good first issue low hanging fruit impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture

Comments

@afharo
Copy link
Member

afharo commented Apr 12, 2022

Given we know that the @elastic/analytics will at least always be imported by core, should we move it to the shared libs?

Originally posted by @pgayvallet in #129927 (comment)

@botelastic botelastic bot added the needs-team Issues missing a team label label Apr 12, 2022
@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Telemetry technical debt Improvement of the software architecture and operational architecture and removed needs-team Issues missing a team label labels Apr 12, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@afharo afharo added loe:small Small Level of Effort impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Apr 12, 2022
@Bamieh
Copy link
Member

Bamieh commented Apr 13, 2022

+1 to adding it as a shared dependency. This can be done at any point so no harm holding until it is actually used in multiple places

@afharo
Copy link
Member Author

afharo commented Apr 13, 2022

AFAIK, it'll only be imported in core for the client initialization and wherever the shippers are asynchronously imported to be registered (cloud and telemetry). The shippers imports are async so they should not affect the page load bundle size. However, it's true that, in the overall picture, we are loading the library 3 times.

@elastic/kibana-operations do you think it's enough of a reason to move it to the shared library?

@pgayvallet
Copy link
Contributor

The shippers imports are async so they should not affect the page load bundle size

FWIW, The shippers are registered during the setup phase, so even if that technically doesn't affect the page load bundle, it would add additional requests during the phase that already is loading the most things.

@spalger
Copy link
Contributor

spalger commented Apr 13, 2022

If it's already loaded by core on every page load then there's little reason to keep it out of the shared-ui-deps.

@afharo
Copy link
Member Author

afharo commented Apr 14, 2022

Thanks for confirming! I'll create a quick follow-up PR next week to address this issue.

@afharo afharo added the good first issue low hanging fruit label May 17, 2022
@afharo afharo changed the title Add @elastic/analytics to the shared libs to test any bundle size improvements Add @kbn/analytics-client to the shared libs to test any bundle size improvements May 17, 2022
@afharo
Copy link
Member Author

afharo commented Jun 29, 2022

Is this still needed? When we initially created it, it referred to @elastic/analytics. That library included the built-in shippers, so it was loaded in 3 places: core, telemetry and cloud.

As we split the package into 3: @kbn/analytics-client (only used in core) and 2 shippers (one for telemetry and another one for cloud). I think adding @kbn/analytics-client to shared-ui-deps won't bring any improvement.

The shippers depend on some types of @kbn/analytics-client, but, AFAIK, that doesn't affect the bundle size because types are removed from the end bundles.

If you think it's still worth giving it a shot, I can try to put a quick PR to test if there's any benefit. What do you think?

@pgayvallet
Copy link
Contributor

It seems fine then, we can probably close this

@afharo
Copy link
Member Author

afharo commented Jun 29, 2022

I'll close then. Please, feel free to reopen if you think it's still worth trying.

@afharo afharo closed this as not planned Won't fix, can't repro, duplicate, stale Jun 29, 2022
@afharo afharo closed this as not planned Won't fix, can't repro, duplicate, stale Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Telemetry good first issue low hanging fruit impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

5 participants