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

[Infra UI] Avoid eager async imports in metric alert registrations #123285

Merged
merged 11 commits into from
Jan 26, 2022

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Jan 18, 2022

📝 Summary

This reverts the eager async imports introduced in #119557 to make sure most chunks are loaded on-demand. In doing so it untangles a web of forbidden imports across the public and server directories which were flagged as linter exceptions.

closes #122941

🕵️‍♀️ Review notes

  • There seems to have been quite some duplication between types in the server and common, so most forbidden imports could be removed by importing from common instead. Where the type didn't yet exist in common, this moves it there.
  • The core of this PR is to make sure no infra.chunk.N.js bundles are loaded on the login screen, but keep the rule creation fly-outs working.

@weltenwort weltenwort added bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 v7.17.0 v7.16.4 v8.0.1 labels Jan 18, 2022
@weltenwort weltenwort self-assigned this Jan 18, 2022
@weltenwort weltenwort marked this pull request as ready for review January 19, 2022 13:25
@weltenwort weltenwort requested a review from a team as a code owner January 19, 2022 13:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@Kerry350 Kerry350 self-requested a review January 25, 2022 10:17
@weltenwort
Copy link
Member Author

looks like an unrelated failure of the CI infrastructure

@weltenwort
Copy link
Member Author

@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
infra 923 920 -3

Async chunks

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

id before after diff
infra 997.4KB 926.6KB -70.8KB

Page load bundle

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

id before after diff
infra 62.2KB 90.5KB +28.3KB
Unknown metric groups

async chunk count

id before after diff
infra 14 13 -1

ESLint disabled line counts

id before after diff
infra 151 127 -24

Total ESLint disabled count

id before after diff
infra 158 134 -24

History

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

cc @weltenwort

@weltenwort
Copy link
Member Author

weltenwort commented Jan 25, 2022

To put the bundle size changes into perspective, here are the changes from the original PR that caused the unfortunate loading behavior:

Async chunks

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

id before after diff
infra 928.7KB 991.0KB +62.3KB

Page load bundle

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

id before after diff
infra 88.3KB 62.2KB -26.1KB

@Kerry350
Copy link
Contributor

Sorry for the delay on this.

Functionally everything looks good.

Before:

Screenshot 2022-01-26 at 14 20 30

After:

Screenshot 2022-01-26 at 14 37 58

Just looking through the code now.

Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

Thank you for untangling all of this ♥️

This cuts a good number of the import changes needed for #58002 (the last issue for #58003).

@weltenwort weltenwort merged commit 629c602 into elastic:main Jan 26, 2022
@weltenwort weltenwort deleted the infra-ui-fix-lazy-public-imports branch January 26, 2022 15:30
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
7.17 Backport failed because of merge conflicts
7.16 Backport failed because of merge conflicts

You might need to backport the following PRs to 7.16:
- [Metrics UI] Fix OR logic on redundant groupBy detection (#116695)
8.0 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.0:
- Revert "[Metrics UI] Fix OR logic on redundant groupBy detection (#116695)" (#116820)
- [Metrics UI] Fix OR logic on redundant groupBy detection (#116695)
- [Metrics UI] Add inventory alert preview (#68909)

How to fix

Re-run the backport manually:

node scripts/backport --pr 123285

Questions ?

Please refer to the Backport tool documentation

@weltenwort
Copy link
Member Author

weltenwort commented Jan 26, 2022

These manual backports will take a while because many of the prior code changes have not been backported cleanly and/or were partially reverted.

weltenwort added a commit to weltenwort/kibana that referenced this pull request Jan 26, 2022
…lastic#123285)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 629c602)

# Conflicts:
#	x-pack/plugins/infra/public/alerting/inventory/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/inventory/components/metric.tsx
#	x-pack/plugins/infra/public/alerting/metric_anomaly/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_row.tsx
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/evaluate_condition.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/lib/calculate_from_based_on_metric.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/evaluate_rule.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_rule_type.ts
#	x-pack/test/api_integration/apis/metrics_ui/inventory_threshold_alert.ts
weltenwort added a commit that referenced this pull request Jan 26, 2022
…123285) (#123868)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 629c602)

# Conflicts:
#	x-pack/plugins/infra/public/alerting/inventory/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/inventory/components/metric.tsx
#	x-pack/plugins/infra/public/alerting/metric_anomaly/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_row.tsx
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/evaluate_condition.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/lib/calculate_from_based_on_metric.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/evaluate_rule.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_rule_type.ts
#	x-pack/test/api_integration/apis/metrics_ui/inventory_threshold_alert.ts
weltenwort added a commit to weltenwort/kibana that referenced this pull request Jan 27, 2022
…lastic#123285)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 629c602)

# Conflicts:
#	x-pack/plugins/infra/public/alerting/inventory/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/inventory/components/expression_chart.tsx
#	x-pack/plugins/infra/public/alerting/inventory/components/metric.tsx
#	x-pack/plugins/infra/public/alerting/inventory/index.ts
#	x-pack/plugins/infra/public/alerting/metric_anomaly/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_row.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/index.ts
#	x-pack/plugins/infra/public/plugin.ts
#	x-pack/plugins/infra/server/features.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/evaluate_condition.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/lib/calculate_from_based_on_metric.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_anomaly/metric_anomaly_executor.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/evaluate_alert.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/metric_query.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts
#	x-pack/test/api_integration/apis/metrics_ui/inventory_threshold_alert.ts
#	x-pack/test/api_integration/apis/metrics_ui/metric_threshold_alert.ts
weltenwort added a commit to weltenwort/kibana that referenced this pull request Jan 27, 2022
…lastic#123285)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
weltenwort added a commit that referenced this pull request Jan 31, 2022
…ions (#123285) (#123967)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
weltenwort added a commit to weltenwort/kibana that referenced this pull request Jan 31, 2022
…lastic#123285)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/infra/public/alerting/inventory/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/inventory/components/expression_chart.tsx
#	x-pack/plugins/infra/public/alerting/inventory/components/metric.tsx
#	x-pack/plugins/infra/public/alerting/inventory/index.ts
#	x-pack/plugins/infra/public/alerting/metric_anomaly/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_row.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/index.ts
#	x-pack/plugins/infra/public/plugin.ts
#	x-pack/plugins/infra/server/features.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/evaluate_condition.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/lib/calculate_from_based_on_metric.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_anomaly/metric_anomaly_executor.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/evaluate_alert.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/metric_query.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts
#	x-pack/test/api_integration/apis/metrics_ui/inventory_threshold_alert.ts
#	x-pack/test/api_integration/apis/metrics_ui/metric_threshold_alert.ts
weltenwort added a commit that referenced this pull request Jan 31, 2022
…123285) (#124177)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/infra/public/alerting/inventory/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/inventory/components/expression_chart.tsx
#	x-pack/plugins/infra/public/alerting/inventory/components/metric.tsx
#	x-pack/plugins/infra/public/alerting/inventory/index.ts
#	x-pack/plugins/infra/public/alerting/metric_anomaly/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_row.tsx
#	x-pack/plugins/infra/public/alerting/metric_threshold/index.ts
#	x-pack/plugins/infra/public/plugin.ts
#	x-pack/plugins/infra/server/features.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/evaluate_condition.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts
#	x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/lib/calculate_from_based_on_metric.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_anomaly/metric_anomaly_executor.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/evaluate_alert.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/metric_query.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts
#	x-pack/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts
#	x-pack/test/api_integration/apis/metrics_ui/inventory_threshold_alert.ts
#	x-pack/test/api_integration/apis/metrics_ui/metric_threshold_alert.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.16.4 v7.17.0 v8.0.1 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infra UI] Plugin registrations pull in UI components too eagerly
5 participants