-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
ES client : use the new type definitions #83808
Conversation
@elasticmachine merge upstream |
merge conflict between base and head |
List of incompatibilities
from https://github.com/elastic/kibana/pull/78413/files#diff-dface0d15abee0d1b6332a9651bd8999d17d724718b8f583a6a348906ef3e4d8R674-R675 So the example with
Side notes
import type { types } from '@elastic/elasticsearch';
const types = ... // error! |
@delvedor Not sure if you saw the feedback above (or I missed a conversation somewhere). How do we anticipate fixing these issues? |
@joshdover yes, I did. Sorry for not writing before. |
I've done another pass and pushed my progress to this branch. There are several issues still, but I agree that we do not need to solve all of them to move forward with adopting this. I've fixed or worked around all of these issues in the core code to get type checking passing in that area. There are still several hundred errors in x-pack and ~30 in OSS code. It would be best to have individual teams resolve the remaining issues. Before we do that, I think we should discuss & resolve the issues in the first two sections below: Questions / Discussion
In general, there are many response keys in various APIs that are only present when certain request parameters are set. Ideally, we could leverage function overloading in TypeScript to return a more accurate response type based on which input parameters are specified. This probably won't be trivial to accomplish, but would very powerful to implement if possible. Major issues (should solve before merging existing PR)
Minor issues
|
@@ -143,7 +143,9 @@ async function deleteIndexTemplates({ client, log, obsoleteIndexTemplatePattern | |||
return; | |||
} | |||
|
|||
const { body: templates } = await client.cat.templates<Array<{ name: string }>>({ | |||
const { | |||
body: { records: templates }, |
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'm not sure we should be using this records
key in the response or not. Needs to be tested here in other usages of the cat API.
I did another update and round of updates. Was able to remove a lot of At this point, the major blocker to continuing is Task Manager's types in Feedback on the elastic/elasticsearch types@delvedor Here is my current feedback based on Core's usage of the client and a few other spots that get picked up by our type building process. Major problemsThese issues need to be fixed before we can merge this:
Minor problemsThese issues are less important and can be avoided mostly by using
|
Thanks for the ping, @joshdover. I've added an item in our project board to help out here. Someone on the team will be in touch soon. |
I've pushed a commit with fixes to support Task Manager, but there are a couple of issues in the typing which have forced a bunch of
|
I've done another round of updates and things are looking even better. I still need to comb through the Feedback on client types for @delvedor
Plugins that we need help withRight now our type check is held back from running the full type check by any plugins that have migrated to TS references, so this list only includes type errors for plugins that have migrated to TS references. There are likely plenty more errors that we will be able to see only once we have fixed these first. We need someone from each of these teams to take a look at their areas, prioritized by number of errors (most to least):
Steps
spaces
security
task_manager
event_log
saved_objects_tagging
lens
data_enhanced
maps
searchprofiler
|
I'll look into the Search Profiler error. |
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.
infra
plugin changes LGTM
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.
LGTM, User experience and Uptime changes !!
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.
Did review from our end for detections engine. LGTM for us, thank you for all the good notes and good comments along the way for us to do follow ups.
Also thank you for treading some lightly in the logic but still fixing a few things along the way, we appreciate it.
})); | ||
|
||
// @ts-expect-error Anomaly is not assignable to EcsAnomaly |
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.
cc @rylnd (FYI)
@@ -34,7 +34,7 @@ export const getThresholdBucketFilters = async ({ | |||
|
|||
bucket.terms.forEach((term) => { | |||
if (term.field != null) { | |||
(filter.bool.filter as ESFilter[]).push({ | |||
(filter.bool!.filter as ESFilter[]).push({ |
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.
cc @madirey
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.
This one is fine, as filter.bool
is hard-coded directly above this. So we definitely won't see any change in behavior and this should work as expected.
@@ -60,6 +60,7 @@ export const getSignalsIndicesInRange = async ({ | |||
'@timestamp': { | |||
gte: from, | |||
lte: 'now', | |||
// @ts-expect-error format doesn't exist in RangeQuery |
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.
cc @rylnd
if (doc._source.event != null && doc._source.event instanceof Object) { | ||
return { ...doc._source.event, kind: 'signal' }; | ||
if (doc._source?.event != null && doc._source?.event instanceof Object) { | ||
return { ...doc._source!.event, kind: 'signal' }; |
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.
@@ -25,5 +24,5 @@ export const buildEventTypeSignal = (doc: BaseSignalHit): object => { | |||
* @param doc The document which might be a signal or it might be a regular log | |||
*/ | |||
export const isEventTypeSignal = (doc: BaseSignalHit): boolean => { | |||
return doc._source.signal?.rule?.id != null && typeof doc._source.signal?.rule?.id === 'string'; | |||
return doc._source?.signal?.rule?.id != null && typeof doc._source?.signal?.rule?.id === '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.
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.
_source
is optional in the new type definitions. Hopefully, it will be fixed in elastic/elasticsearch-specification#197
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 is optional, _source
can be set to false
, and maybe we want to encourage use of the fields API instead anyway?
allow_no_indices: true, | ||
body: { | ||
terminate_after: 1, | ||
}, |
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.
cc @spong
@@ -730,6 +731,7 @@ export const mergeSearchResults = (searchResults: SignalSearchResponse[]) => { | |||
total: newShards.total + existingShards.total, | |||
successful: newShards.successful + existingShards.successful, | |||
failed: newShards.failed + existingShards.failed, | |||
// @ts-expect-error @elastic/elaticsearch skipped is optional in ShardStatistics |
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.
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.
Stack Monitoring looking good. Thank you for ding this!
* Use client from branch * Get type checking working in core * Fix types in other plugins * Update client types + remove type errors from core * migrate Task Manager Elasticsearch typing from legacy library to client library * use SortOrder instead o string in alerts * Update client types + fix core type issues * fix maps ts errors * Update Lens types * Convert Search Profiler body from a string to an object to conform to SearchRequest type. * Fix SOT types * Fix/mute Security/Spaces plugins type errors. * Fix bootstrap types * Fix painless_lab * corrected es typing in Event Log * Use new types from client for inferred search responses * Latest type defs * Integrate latest type defs for APM/UX * fix core errors * fix telemetry errors * fix canvas errors * fix data_enhanced errors * fix event_log errors * mute lens errors * fix or mute maps errors * fix reporting errors * fix security errors * mute errors in task_manager * fix errors in telemetry_collection_xpack * fix errors in data plugins * fix errors in alerts * mute errors in index_management * fix task_manager errors * mute or fix lens errors * fix upgrade_assistant errors * fix or mute errors in index_lifecycle_management * fix discover errors * fix core tests * ML changes * fix core type errors * mute error in kbn-es-archiver * fix error in data plugin * fix error in telemetry plugin * fix error in discover * fix discover errors * fix errors in task_manager * fix security errors * fix wrong conflict resolution * address errors with upstream code * update deps to the last commit * remove outdated comments * fix core errors * fix errors after update * adding more expect errors to ML * pull the lastest changes * fix core errors * fix errors in infra plugin * fix errors in uptime plugin * fix errors in ml * fix errors in xpack telemetry * fix or mute errors in transform * fix errors in upgrade assistant * fix or mute fleet errors * start fixing apm errors * fix errors in osquery * fix telemetry tests * core cleanup * fix asMutableArray imports * cleanup * data_enhanced cleanup * cleanup events_log * cleaup * fix error in kbn-es-archiver * fix errors in kbn-es-archiver * fix errors in kbn-es-archiver * fix ES typings for Hit * fix SO * fix actions plugin * fix fleet * fix maps * fix stack_alerts * fix eslint problems * fix event_log unit tests * fix failures in data_enhanced tests * fix test failure in kbn-es-archiver * fix test failures in index_pattern_management * fixing ML test * remove outdated comment in kbn-es-archiver * fix error type in ml * fix eslint errors in osquery plugin * fix runtime error in infra plugin * revert changes to event_log cluser exist check * fix eslint error in osquery * fixing ML endpoint argument types * fx types * Update api-extractor docs * attempt fix for ese test * Fix lint error * Fix types for ts refs * Fix data_enhanced unit test * fix lens types * generate docs * Fix a number of type issues in monitoring and ml * fix triggers_actions_ui * Fix ILM functional test * Put search.d.ts typings back * fix data plugin * Update typings in typings/elasticsearch * Update snapshots * mute errors in task_manager * mute fleet errors * lens. remove unnecessary ts-expect-errors * fix errors in stack_alerts * mute errors in osquery * fix errors in security_solution * fix errors in lists * fix errors in cases * mute errors in search_examples * use KibanaClient to enforce promise-based API * fix errors in test/ folder * update comment * fix errors in x-pack/test folder * fix errors in ml plugin * fix optional fields in ml api_integartoon tests * fix another casting problem in ml tests * fix another ml test failure * fix fleet problem after conflict resolution * rollback changes in security_solution. trying to fix test * Update type for discover rows * uncomment runtime_mappings as its outdated * address comments from Wylie * remove eslint error due to any * mute error due to incompatibility * Apply suggestions from code review Co-authored-by: John Schulz <[email protected]> * fix type error in lens tests * Update x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts Co-authored-by: Alison Goryachev <[email protected]> * Update x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.test.ts Co-authored-by: Alison Goryachev <[email protected]> * update deps * fix errors in core types * fix errors for the new elastic/elasticsearch version * remove unused type * remove unnecessary manual type cast and put optional chaining back * ML: mute Datafeed is missing indices_options * Apply suggestions from code review Co-authored-by: Josh Dover <[email protected]> * use canary pacakge instead of git commit Co-authored-by: Josh Dover <[email protected]> Co-authored-by: Josh Dover <[email protected]> Co-authored-by: Gidi Meir Morris <[email protected]> Co-authored-by: Nathan Reese <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: CJ Cenizal <[email protected]> Co-authored-by: Aleh Zasypkin <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]> Co-authored-by: restrry <[email protected]> Co-authored-by: James Gowdy <[email protected]> Co-authored-by: John Schulz <[email protected]> Co-authored-by: Alison Goryachev <[email protected]> # Conflicts: # package.json # src/core/server/saved_objects/migrationsv2/integration_tests/migration.test.ts # src/plugins/data/server/autocomplete/value_suggestions_route.ts # src/plugins/discover/public/application/angular/doc_table/create_doc_table_react.tsx # x-pack/plugins/apm/server/lib/services/get_service_dependencies/get_destination_map.ts # x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/evaluate_condition.ts # x-pack/plugins/security/server/authentication/api_keys/api_keys.ts # yarn.lock
* ES client : use the new type definitions (#83808) * Use client from branch * Get type checking working in core * Fix types in other plugins * Update client types + remove type errors from core * migrate Task Manager Elasticsearch typing from legacy library to client library * use SortOrder instead o string in alerts * Update client types + fix core type issues * fix maps ts errors * Update Lens types * Convert Search Profiler body from a string to an object to conform to SearchRequest type. * Fix SOT types * Fix/mute Security/Spaces plugins type errors. * Fix bootstrap types * Fix painless_lab * corrected es typing in Event Log * Use new types from client for inferred search responses * Latest type defs * Integrate latest type defs for APM/UX * fix core errors * fix telemetry errors * fix canvas errors * fix data_enhanced errors * fix event_log errors * mute lens errors * fix or mute maps errors * fix reporting errors * fix security errors * mute errors in task_manager * fix errors in telemetry_collection_xpack * fix errors in data plugins * fix errors in alerts * mute errors in index_management * fix task_manager errors * mute or fix lens errors * fix upgrade_assistant errors * fix or mute errors in index_lifecycle_management * fix discover errors * fix core tests * ML changes * fix core type errors * mute error in kbn-es-archiver * fix error in data plugin * fix error in telemetry plugin * fix error in discover * fix discover errors * fix errors in task_manager * fix security errors * fix wrong conflict resolution * address errors with upstream code * update deps to the last commit * remove outdated comments * fix core errors * fix errors after update * adding more expect errors to ML * pull the lastest changes * fix core errors * fix errors in infra plugin * fix errors in uptime plugin * fix errors in ml * fix errors in xpack telemetry * fix or mute errors in transform * fix errors in upgrade assistant * fix or mute fleet errors * start fixing apm errors * fix errors in osquery * fix telemetry tests * core cleanup * fix asMutableArray imports * cleanup * data_enhanced cleanup * cleanup events_log * cleaup * fix error in kbn-es-archiver * fix errors in kbn-es-archiver * fix errors in kbn-es-archiver * fix ES typings for Hit * fix SO * fix actions plugin * fix fleet * fix maps * fix stack_alerts * fix eslint problems * fix event_log unit tests * fix failures in data_enhanced tests * fix test failure in kbn-es-archiver * fix test failures in index_pattern_management * fixing ML test * remove outdated comment in kbn-es-archiver * fix error type in ml * fix eslint errors in osquery plugin * fix runtime error in infra plugin * revert changes to event_log cluser exist check * fix eslint error in osquery * fixing ML endpoint argument types * fx types * Update api-extractor docs * attempt fix for ese test * Fix lint error * Fix types for ts refs * Fix data_enhanced unit test * fix lens types * generate docs * Fix a number of type issues in monitoring and ml * fix triggers_actions_ui * Fix ILM functional test * Put search.d.ts typings back * fix data plugin * Update typings in typings/elasticsearch * Update snapshots * mute errors in task_manager * mute fleet errors * lens. remove unnecessary ts-expect-errors * fix errors in stack_alerts * mute errors in osquery * fix errors in security_solution * fix errors in lists * fix errors in cases * mute errors in search_examples * use KibanaClient to enforce promise-based API * fix errors in test/ folder * update comment * fix errors in x-pack/test folder * fix errors in ml plugin * fix optional fields in ml api_integartoon tests * fix another casting problem in ml tests * fix another ml test failure * fix fleet problem after conflict resolution * rollback changes in security_solution. trying to fix test * Update type for discover rows * uncomment runtime_mappings as its outdated * address comments from Wylie * remove eslint error due to any * mute error due to incompatibility * Apply suggestions from code review Co-authored-by: John Schulz <[email protected]> * fix type error in lens tests * Update x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts Co-authored-by: Alison Goryachev <[email protected]> * Update x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.test.ts Co-authored-by: Alison Goryachev <[email protected]> * update deps * fix errors in core types * fix errors for the new elastic/elasticsearch version * remove unused type * remove unnecessary manual type cast and put optional chaining back * ML: mute Datafeed is missing indices_options * Apply suggestions from code review Co-authored-by: Josh Dover <[email protected]> * use canary pacakge instead of git commit Co-authored-by: Josh Dover <[email protected]> Co-authored-by: Josh Dover <[email protected]> Co-authored-by: Gidi Meir Morris <[email protected]> Co-authored-by: Nathan Reese <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: CJ Cenizal <[email protected]> Co-authored-by: Aleh Zasypkin <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]> Co-authored-by: restrry <[email protected]> Co-authored-by: James Gowdy <[email protected]> Co-authored-by: John Schulz <[email protected]> Co-authored-by: Alison Goryachev <[email protected]> # Conflicts: # package.json # src/core/server/saved_objects/migrationsv2/integration_tests/migration.test.ts # src/plugins/data/server/autocomplete/value_suggestions_route.ts # src/plugins/discover/public/application/angular/doc_table/create_doc_table_react.tsx # x-pack/plugins/apm/server/lib/services/get_service_dependencies/get_destination_map.ts # x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/evaluate_condition.ts # x-pack/plugins/security/server/authentication/api_keys/api_keys.ts # yarn.lock * fix file broken during conflict resolution * fix lint errors and test failure Co-authored-by: Tomas Della Vedova <[email protected]>
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Closes and opens alerts.Closing alerts Closes and opens alertsStack Trace
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
closes #88668
The JavaScript client will offer a comprehensive type definition of the Elasticsearch API.
PR in the client library: elastic/elasticsearch-js#1358
Notes for reviewers
If your code still uses the legacy Elasticsearch client provided by the Kibana Core, please migrate to the new Elasticsearch client using this instruction.
If your code already leverages the new Elasticsearch client, please, in a follow-up PR, remove all type definitions imported from the legacy 'elasticseach
client library and manually maintained type definitions. Instead, use typings provided from
@elastic/elasticsearch` client:Also, please review all the
@elastic/elasticsearch
library type generics used in your code. Most of them either unnecessary or should be refactored:We are at the very beginning of supporting type definitions in the ES client library. Keep in mind, during the code review process, that provided types aren't complete and might contain some mistakes.
If you spot any problems in the imported typings, please mute them with
@ts-expect-error @elastic/elasticsearch
instruction and report them to https://github.com/elastic/elastic-client-generator/issues/new/chooseOnce the type errors are fixed in the upstream library, Kibana will receive the update, and
@ts-expect-error @elastic/elasticsearch
should be deleted.If you see
@ts-expect-error
in this PR (instead of@ts-expect-error @elastic/elasticsearch
), it's likely to be a problem in the Kibana code that should be addressed by code owners later. If you are sure, it's a problem in the Client code, please, report the problem to https://github.com/elastic/elastic-client-generator/issues/new/chooseList of known problems
Moved to elastic/elasticsearch-specification#197
Plugin API changes
In the past releases, Kibana Platform provided an Elasticsearch client that didn't have any response typings out of the box.
From now on, the Elasticsearch client is provided with enhanced response type definitions. If your code already leverages the new Elasticsearch client, remove all type definitions imported from the legacy
elasticseach
client library and all the manually maintained type definitions. Instead, use typings provided from the@elastic/elasticsearch
client:Also, please review all the
@elastic/elasticsearch
library type generics used in your code. Most of them either unnecessary or should be refactored: