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

[pkgs/peggy] automatically transform peggy files with babel-register and webpack #145615

Merged

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Nov 17, 2022

In order to get us closer to the developer experience we want for packages, we are trying to move package builds out of bazel and instead we want to build files on demand. In the case of .peggy files this means importing them directly and teaching babel/jest/webpack how to handle these imports by automatically transpiling and caching the results.

This change does just that, adding a @kbn/peggy package which wraps peggy for types, and also adds support for defining peggy config adjacent to a peggy grammar file in a ${basename}.config.json file. This file will be parsed and used to configure things like allowedStartRules as described in the peggy docs.

This PR also implements @kbn/peggy-loader which uses @kbn/peggy to transpile peggy files in webpack, and a peggy transform for both Jest and our custom babel register hook.

@spalger spalger force-pushed the implement/babel-register-hook-for-peggy-files branch 6 times, most recently from 697431e to 7ee2e8e Compare November 19, 2022 02:43
@spalger spalger force-pushed the implement/babel-register-hook-for-peggy-files branch from 7ee2e8e to 0214607 Compare November 19, 2022 02:55
@spalger spalger added Team:Operations Team label for Operations Team release_note:skip Skip the PR/issue when compiling release notes labels Nov 21, 2022
@spalger spalger marked this pull request as ready for review November 21, 2022 16:16
@spalger spalger requested review from a team as code owners November 21, 2022 16:16
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

mind if we add the ci:build-storybooks and ci:build-canvas-shareable-runtime labels?

src/dev/build/tasks/build_packages_task.ts Outdated Show resolved Hide resolved
packages/kbn-peggy-loader/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Canvas sharable runtime webpack config change LGTM.

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

lgtm

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimelion 45 46 +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
@kbn/peggy - 10 +10
@kbn/test 220 221 +1
total +11

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id before after diff
@kbn/interpreter 13 12 -1
@kbn/peggy - 1 +1
total -0

Async chunks

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

id before after diff
canvas 1.0MB 1.0MB -23.0B
expressions 28.4KB 28.4KB +56.0B
lens 1.3MB 1.3MB -19.0B
presentationUtil 130.8KB 130.7KB -80.0B
visTypeTimelion 74.5KB 74.6KB +93.0B
total +27.0B

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5039 +5039
total size - 7.9MB +7.9MB

Page load bundle

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

id before after diff
expressions 98.6KB 98.5KB -74.0B
kbnUiSharedDeps-srcJs 4.1MB 4.1MB -124.0B
total -198.0B
Unknown metric groups

API count

id before after diff
@kbn/interpreter 50 52 +2
@kbn/peggy - 21 +21
@kbn/test 264 265 +1
total +24

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
@kbn/peggy-loader - 1 +1
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 443 449 +6
total +21

Total ESLint disabled count

id before after diff
@kbn/peggy-loader - 1 +1
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 520 526 +6
total +22

History

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

@spalger spalger merged commit cfdb855 into elastic:main Nov 22, 2022
@spalger spalger deleted the implement/babel-register-hook-for-peggy-files branch November 22, 2022 18:25
@kibanamachine kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Nov 22, 2022
lukasolson added a commit that referenced this pull request Apr 27, 2024
## Summary

Resolves #143335.

Some history: A similar issue was reported a few years back
(#76811). The solution
(#93319) was to use the `--cache`
PEG.js [parameter](https://pegjs.org/documentation#generating-a-parser)
when generating the parser. Back when this was added, we were still
manually building the parser on demand when it was changed. Eventually
we added support for dynamically building the parser during the build
process (#145615). I'm not sure
where along the process the `cache` parameter got lost but it didn't
appear to be used when we switched.

This PR re-adds this parameter which increases performance considerably
(metrics shown in ops/sec):

```
Before using cache:

  ● kuery AST API › fromKueryExpression › performance › with simple expression
    Received:   7110.68990544415

  ● kuery AST API › fromKueryExpression › performance › with complex expression
    Received:   40.51361746242248

  ● kuery AST API › fromKueryExpression › performance › with many subqueries
    Received:   17.071767133068473

After using cache:

  ● kuery AST API › fromKueryExpression › performance › with simple expression
    Received:   8275.49109867502

  ● kuery AST API › fromKueryExpression › performance › with complex expression
    Received:   447.0459218892934

  ● kuery AST API › fromKueryExpression › performance › with many subqueries
    Received:   115852.43643466769
```

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
yuliacech pushed a commit to yuliacech/kibana that referenced this pull request May 3, 2024
## Summary

Resolves elastic#143335.

Some history: A similar issue was reported a few years back
(elastic#76811). The solution
(elastic#93319) was to use the `--cache`
PEG.js [parameter](https://pegjs.org/documentation#generating-a-parser)
when generating the parser. Back when this was added, we were still
manually building the parser on demand when it was changed. Eventually
we added support for dynamically building the parser during the build
process (elastic#145615). I'm not sure
where along the process the `cache` parameter got lost but it didn't
appear to be used when we switched.

This PR re-adds this parameter which increases performance considerably
(metrics shown in ops/sec):

```
Before using cache:

  ● kuery AST API › fromKueryExpression › performance › with simple expression
    Received:   7110.68990544415

  ● kuery AST API › fromKueryExpression › performance › with complex expression
    Received:   40.51361746242248

  ● kuery AST API › fromKueryExpression › performance › with many subqueries
    Received:   17.071767133068473

After using cache:

  ● kuery AST API › fromKueryExpression › performance › with simple expression
    Received:   8275.49109867502

  ● kuery AST API › fromKueryExpression › performance › with complex expression
    Received:   447.0459218892934

  ● kuery AST API › fromKueryExpression › performance › with many subqueries
    Received:   115852.43643466769
```

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
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:build-canvas-shareable-runtime ci:build-storybooks release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants