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

Refactor updateBundleLimits to categorize and sort limits #6504

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Apr 17, 2024

Description

This commit updates the updateBundleLimits function to categorize plugin limits into 'External Plugins' and 'OpenSearch Dashboards Plugins'. Each category is now sorted by size in descending order. This categorization and sorting are aimed at
improving the clarity and manageability of the page load asset size limits.

Issues Resolved

NA

Screenshot

Testing the changes

Changelog

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

This commit updates the `updateBundleLimits` function to categorize plugin limits into
'External Plugins' and 'OpenSearch Dashboards Plugins'. Each category is now sorted
 by size in descending order. This categorization and sorting are aimed at
improving the clarity and manageability of the page load asset size
limits.

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Copy link
Contributor

ℹ️ Manual Changeset Creation Reminder

Please ensure manual commit for changeset file 6504.yml under folder changelogs/fragments to complete this PR.

If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link.

For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool.


const newLimits: SortedLimits = {
pageLoadAssetSize: {
'External Plugins': externalPlugins,
Copy link
Member

Choose a reason for hiding this comment

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

perhaps:

pageLoadAssetSize: {
  external: externalPlugins,
  internal: osdPlugins,
}

Since both categories are technically OpenSearch Dashboards Plugins

existingLimit != null && existingLimit >= metric.value
? existingLimit
: metric.value + DEFAULT_BUDGET;

if (metric.id.includes('Dashboards')) {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I don't think we can use Dashboards I can see that some internal plugins got caught in this filter for example opensearchDashboardsReact.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could filter out results that include "Dashboards" but specifically exclude those that start with "opensearchDashboards"

@@ -93,22 +93,37 @@ export function validateLimitsForAllBundles(log: ToolingLog, config: OptimizerCo
export function updateBundleLimits(log: ToolingLog, config: OptimizerConfig) {
const metrics = getMetrics(log, config);
Copy link
Member

Choose a reason for hiding this comment

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

will need to update validate function as well

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

just a couple comments but i really like this new format easier to read when trying to focus on general memory and size issues

@BionIT
Copy link
Collaborator

BionIT commented Jun 5, 2024

Hi @ananzh, is this PR for 2.15 or 2.16?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants