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

Switch to typescript project references and incremental builds #46773

Closed
6 of 7 tasks
rudolf opened this issue Sep 27, 2019 · 19 comments
Closed
6 of 7 tasks

Switch to typescript project references and incremental builds #46773

rudolf opened this issue Sep 27, 2019 · 19 comments
Assignees
Labels
Feature:Development Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v7.10.0

Comments

@rudolf
Copy link
Contributor

rudolf commented Sep 27, 2019

Developing Typescript with VSCode is currently frustratingly slow:

  • Opening files in another one of Kibana's 24 independant typescript projects sometimes takes > 60s "initializing TS language features"
  • cmd+clicking references within some projects takes > 60s (in my experience test seems to be the slowest)
  • Not all type errors show up under VSCode's "problems"
  • Running a complete typecheck takes > 60s which slows down the development cycle when doing large type refactors

I believe using [typescript references](https://www.typescriptlang.org/docs/handbook/project-references.html and incremental builds we should be able to make the build time much faster, have type errors show up without having to run scripts/type_check and improve type lookups and navigation in VSCode.

The first iteration #72280

Steps

@rudolf rudolf added the Team:Operations Team label for Operations Team label Sep 27, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations

@spalger
Copy link
Contributor

spalger commented Oct 1, 2019

I've tried to do this a couple times but it wasn't very easy, though I agree it's worth putting real effort into.

@trusktr
Copy link

trusktr commented May 12, 2020

This is unrelated to Kibana (I was Googling for Project References information), but when I try running tsc --build --incremental --tsBuildInfoFile ./.buildinfo where --build enabled the new Project References features, it complains error TS5072: Unknown build option '--tsBuildInfoFile'.

I'm not sure, but I think that incremental builds are a default feature of Project References (--build) if I read that that doc page correctly.

EDIT: Ah, yep! This is what I see in watch mode with --build --watch:

[7:43:06 PM] File change detected. Starting incremental compilation...

Note the "incremental" part. So, yeah, it is incremental by default. Cool!

@rudolf
Copy link
Contributor Author

rudolf commented May 12, 2020

I did a quick (and dirty) spike to get incremental builds working for Kibana: #46772

@mshustov
Copy link
Contributor

mshustov commented Jul 9, 2020

@rudolf Could you measure the effect with TS built-in diagnostic tools https://github.com/microsoft/TypeScript/wiki/Performance#investigating-issues?

@rudolf
Copy link
Contributor Author

rudolf commented Jul 9, 2020

@rudolf Could you measure the effect with TS built-in diagnostic tools

Good idea!

@rudolf rudolf added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Jul 9, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@jinmu03
Copy link
Contributor

jinmu03 commented Aug 14, 2020

Combined this issue with #72280

@jinmu03 jinmu03 closed this as completed Aug 14, 2020
@mshustov mshustov reopened this Aug 21, 2020
@mshustov mshustov self-assigned this Aug 21, 2020
@mshustov
Copy link
Contributor

mshustov commented Aug 21, 2020

All tests done on #73924 with TS v4.0.2
TS provides several options to improve perf:

Incremental builds

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#faster-subsequent-builds-with-the---incremental-flag
Can be used with —noEmit flag.
Generates *.tsbuildinfo fail to cache dependency tree.
How it affects type check time of OSS project:

  • incremental: false first pass 33.9s
  • incremental: false second pass 33.8s
Files:             5779
Lines:            783785
Nodes:           2632074
Identifiers:         897987
Symbols:          1750565
Types:            597330
Instantiations:       2072774
Memory used:        1675133K
Assignability cache size:  255152
Identity cache size:     14662
Subtype cache size:      21525
Strict subtype cache size:  18711
I/O Read time:        0.31s
Parse time:          3.19s
ResolveTypeReference time:  0.06s
ResolveModule time:      1.73s
Program time:         5.88s
Bind time:          1.58s
Check time:         26.33s
printTime time:        0.00s
Emit time:          0.00s
Total time:         33.79s
  • incremental: true first pass: 45s
Files:             5779
Lines:            783785
Nodes:           2632074
Identifiers:         897987
Symbols:          1751312
Types:            597411
Instantiations:       2074247
Memory used:        1761445K
Assignability cache size:  255108
Identity cache size:     14662
Subtype cache size:      21512
Strict subtype cache size:  18715
I/O Read time:        0.36s
Parse time:          3.13s
ResolveTypeReference time:  0.04s
ResolveModule time:      1.72s
Program time:         5.93s
Bind time:          1.61s
Check time:         24.13s
transformTime time:     11.41s
commentTime time:       0.30s
printTime time:       13.34s
Emit time:          13.38s
I/O Write time:        0.02s
Total time:         45.05s
  • incremental: true second pass: 7.4s
Files:            5779
Lines:           783785
Nodes:           2632074
Identifiers:        897987
Symbols:          485640
Types:             76
Instantiations:         0
Memory used:        798209K
Assignability cache size:    0
Identity cache size:       0
Subtype cache size:       0
Strict subtype cache size:    0
I/O Read time:        0.32s
Parse time:         3.07s
ResolveTypeReference time:  0.05s
ResolveModule time:     1.61s
Program time:        5.78s
Bind time:          1.65s
Total time:         7.43s
  • incremental: true an exported type changed: 45.6s
  • incremental: true an internal type changed 42.6s

How it affects type build time of the OSS project:

  • incremental: false first pass: 20.8s
  • incremental: true first pass: 24.5s
  • incremental: true second pass: 4.6s
  • incremental: true an internal type changed 33.6s

We can conclude that it speeds up type check and build time if code hasn’t changed. Which is not the case for actively developed projects as Kibana. We could reduce scopes and make projects more focused - make every plugin a separate TS project at least for x-pack plugins, extract oss & x-pack tests in separate ts projects as well. @elastic/kibana-operations Do you envision any problems with this approach?

Project references

https://www.typescriptlang.org/docs/handbook/project-references.html
It turned out that we cannot use project references without building project types for referenced projects (we need to emit at least their type declarations). We hoped that TS from v4 allows this use case with —-noEmit flag but turned out it’s not possible by design https://github.com/microsoft/TypeScript/pull/39122/files#diff-27762b0db782b18c2ddc01669e7dd54fR33
I need to take another stab at this work started in #72280

@spalger
Copy link
Contributor

spalger commented Aug 21, 2020

Thank you so much for doing this research.

We could reduce scopes and make projects more focused - make every plugin a separate TS project at least for x-pack plugins, extract oss & x-pack tests in separate ts projects as well. @elastic/kibana-operations Do you envision any problems with this approach?

This is definitely what I hope we will do, though I don't think it will be easy.

@mshustov
Copy link
Contributor

Ok, I'm going to start with a single plugin and a test project to see what problems we discover along the way.

@mshustov
Copy link
Contributor

mshustov commented Aug 24, 2020

Today I had a stab at composite project integration Kibana repo.

To make them work we should:

It requires starting from a leaf project with no dependencies. It would seem that the Kibana platform is an apparent candidate for this role. But it's not that simple, unfortunately:

  • The platform depends on the data plugin on the server-side. Shouldn't we remove this dependency? Extracting KQL into a separate package, maybe? @elastic/kibana-platform
  • The platform depends on cli/cluster_manager. Can it be moved under src/core/server? @elastic/kibana-operations
  • There are several dependencies on src/legacy. Either we remove them or move them under src/core.
  • The platform depends on legacy/server/kbn_server which has deps on several plugins. These deps can likely be removed since there are no more legacy server consumers in the solutions code.
  • The platform tests depend on test_utils/kbn_server which likewise needs moving under the platform folder.

src/test_utils is another small candidate to move to TS projects, but we need to remove deps on the platform to make it happen.

@pgayvallet
Copy link
Contributor

The platform depends on the data plugin on the server-side. Shouldn't we remove this dependency? Extracting KQL into a separate package, maybe? @elastic/kibana-platform

There was a discussion about this topic when the core->data dependency was actually introduced. Don't exactly remember what the plan was though. @rudolf @elastic/kibana-app-arch does someone remember what our options were?

The platform depends on cli/cluster_manager. Can it be moved under src/core/server?

Let's see what operations have to say about this one, but moving at least the typedefs to core SGTM.

There are several dependencies on src/legacy. Either we remove them or move them under src/core

Lots of them should disappear with the legacy plugin discovery removal + legacy service. Most of the ones remaining should be imports of src/legacy/utils/streams from core. This package should probably move to core until we decide exactly what to do with it.

The platform tests depend on test_utils/kbn_server which likewise needs moving under the platform folder.

src/test_utils is another small candidate to move to TS projects, but we need to remove deps on the platform to make it happen.

Would have hoped that creating test_utils/kbn_server project would be easy, however there are bidirectional dependencies.

From a (very) quick look, it seems to be only from

  • src/test_utils/public/http_test_setup.ts
  • src/test_utils/kbn_server.ts

However src/test_utils/kbn_server.ts also imports from ../legacy/... so not sure of the difficulty to move this to core.

@joshdover
Copy link
Contributor

There was a discussion about this topic when the core->data dependency was actually introduced. Don't exactly remember what the plan was though. @rudolf @elastic/kibana-app-arch does someone remember what our options were?

We opened an issue for this with a high-level plan forward here: #55485
Basically, the proposal is to allow arbitrary query predicates to be passed into the find API. I believe @kobelb has also found that adding this would allow for some perf improvements to some Fleet endpoints that currently need KQL-like functionality.

The platform depends on cli/cluster_manager. Can it be moved under src/core/server?

Let's see what operations have to say about this one, but moving at least the typedefs to core SGTM.

+1

The platform tests depend on test_utils/kbn_server which likewise needs moving under the platform folder.

I wonder if we should just re-write this utility in Core and adapt the existing usages of it to this new test harness. I only see 18 usages of it, so I don't think it will be too difficult.

@spalger
Copy link
Contributor

spalger commented Aug 24, 2020

  • The platform depends on cli/cluster_manager. Can it be moved under src/core/server? @elastic/kibana-operations

My instinct is to decouple the core from the CLI rather than couple them further. It might be challenging because the CLI requires some config, so maybe we need to move it into core temporarily until we can decouple the CLI and keep it indenedent.

@kobelb
Copy link
Contributor

kobelb commented Aug 24, 2020

Basically, the proposal is to allow arbitrary query predicates to be passed into the find API

My one concern is that allowing consumers to directly write the query predicates ends up leaking the way we map from a saved-object to the Elasticsearch document which represents it. This is the one benefit that we get from KQL, it's structured in such a manner that we can rewrite the query. For example, a consumer of the find API shouldn't need to know that we put everything under attributes in a field which matches the type. You can't just write the following to look for a dashboard with a specific :

"query": {
    "term": {
      "attributes.description": {
        "value": "foo"
      }
    }
  }

you have to write

"query": {
    "term": {
      "dashboard.description": {
        "value": "foo"
      }
    }
  }

I believe @kobelb has also found that adding this would allow for some perf improvements to some Fleet endpoints that currently need KQL-like functionality.

FWIW, we found a way around this by allowing Fleet endpoints to build the KQL manually and not parse it from a string #75693

@mshustov
Copy link
Contributor

WIP PR moving src/test_utils in to a separate project: mshustov#6

@mshustov
Copy link
Contributor

mshustov commented Sep 1, 2020

PR extracting src/test_utils into TS project #76082
Next steps:

  • introduce project references for platform code under src/core
  • introduce project references for src/plugins/kibana_utils
  • document migration path for solution teams

src/core migration

To migrate to TS project references, we need to comply with several requirements:

  • src/core projects imports only from explicitly defined TS projects (src/test_utils)
  • src/core compiles all the TS files it imports

There are a few blockers at the moment:

CLI

We can fix this by changing import to *js file temporarily until we remove this dependency.

Data plugin

Already has issue #55485

Legacy Utils

Migrate to KP stream helpers #76397

JS deps

They aren't hard requirements, but we should remove them eventually:

Legacy Config

Legacy server

Legacy plugins

tracked as

yarn add -D dependency-cruiser
./node_modules/.bin/dependency-cruiser --init
rm deps.txt && ./node_modules/.bin/dependency-cruiser src/core/ -x "^(node_modules|packages)" -T text | grep -v '→ *src\/core*' > deps.txt

@mshustov
Copy link
Contributor

We added tooling, docs, and examples. Now it's up to the plugins to complete the migration. I created a migration meta issue #80508 and going to close the current one. Thanks to everyone involved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Development Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v7.10.0
Projects
None yet
Development

No branches or pull requests

10 participants