-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
TypeScript project references for APM #90049
Conversation
References elastic#80508. References elastic#81003.
⏳ Build in-progress, with failures
Failed CI Steps
History
To update your PR or re-run it, just comment with: |
retest |
@elasticmachine merge upstream |
.gitignore
Outdated
@@ -61,9 +61,6 @@ npm-debug.log* | |||
.ci/bash_standard_lib.sh | |||
.gradle | |||
|
|||
# apm plugin | |||
/x-pack/plugins/apm/tsconfig.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look, and completionInfo() is still twice as fast with the optimized tsconfig, and project start up time also seems to be slower. I'm not sure if I'm doing something wrong. What do you think about leaving this in for now and removing it later if we feel like it's no longer worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would be better served by concentrating any effort on further performance kibana-wide, rather than having a different setup for APM. I'm ok with leaving it in, but don't think we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in not spending any effort to improve this. So if it's causing issues let's remove it. But it worked well before, and I would like us to keep it around for a bit longer to compare the two different setups.
@elasticmachine merge upstream |
[SPAN_SUBTYPE]: string; | ||
'span.destination.service.resource': string; | ||
'span.type': string; | ||
'span.subtype': string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not possible to use the variables anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bug in 4.1 that will be fixed in 4.2. There's some explanation in the comment above this code.
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Nathan!
Backport result
|
This reverts commit f980405.
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co> Co-authored-by: Nathan L Smith <nathan.smith@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
There are a few workarounds for a bug in TypeScript 4.2, which can be removed once we've upgraded to that (we're tracking this in #90788.)
Because we're emitting declarations a few extra interfaces need to be exported.
The optimization scripts have been removed.
The scripts directory previously had its own tsconfig, but now everything is under one config, so a few changes were made there.
Change all spelling of "throuput" to "throughput"
Fixes #81003. References #80508.