-
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
[Enterprise Search] [Search application] List indices fetch indices stats from ES directly #153696
[Enterprise Search] [Search application] List indices fetch indices stats from ES directly #153696
Conversation
@@ -20,11 +20,14 @@ export interface EnterpriseSearchEngine { | |||
} | |||
|
|||
export interface EnterpriseSearchEngineDetails { | |||
indices: EnterpriseSearchEngineIndex[]; | |||
indices: 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.
indices are of type string[] from ES but the hydrated indices is of type EnterpriseSearchEngineIndex[]. Hence added EnterpriseSearchEngineDetailsResponse
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 type was only needed because the get engine & list engines results differed. since this is returning indices: string[];
now we can just use EnterpriseSearchEngine
as the types are the same.
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.
Nice and clean! One nitpick, could you please update the PR title to be more descriptive of the change? Thank you!
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.
Nice job! Just one spelling nitpick
metric: ['docs'], | ||
}); | ||
const indicesWithStats = indicesNames.map((indexName: string) => { | ||
const indiceStats = indicesStats[indexName]; |
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.
nitpick(non-blocking): indiceStats
should be indicesStats
(or maybe indexStats
).
8d3baf7
to
6916094
Compare
@elasticmachine merge upstream |
updated_at_millis: number; | ||
name: string; | ||
} | ||
|
||
export interface EnterpriseSearchEngineDetailsResponse |
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.
introducing this type doesn't make sense to me. I think we should update EnterpriseSearchEngineDetails
with indices: EnterpriseSearchEngineIndex[];
instead of introducing another interface here.
d30f363
to
1e3a2f3
Compare
1e3a2f3
to
705935b
Compare
@elasticmachine merge upstream |
@@ -14,15 +14,9 @@ export interface EnterpriseSearchEnginesResponse { | |||
} | |||
|
|||
export interface EnterpriseSearchEngine { |
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.
are we hydrating indices
with the full index data for the list
request?
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.
List all engines and GET a engine request returns list indices names in 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.
We are fetching indices data (stats
) from ES directly. Logic added in this file.
https://github.com/elastic/kibana/blob/7eb14bf51a9912f4bbf82afe8018d2ee3b3a9120/x-pack/plugins/enterprise_search/server/lib/engines/fetch_indices_stats.ts
updated_at_millis: number; | ||
} | ||
|
||
export interface EnterpriseSearchEngineDetails { |
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.
We'll still need this type to be different from EnterpriseSearchEngine
since the list and get responses return different data. List results has indices: string[]
and should use EnterpriseSearchEngine
and the get for a single search application will need to return a EnterpriseSearchEngineDetails
which has the indices hydrated.
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.
Yeah it make sense.
General question:
If we define indices: EnterpriseSearchEngineIndex[]
in EnterpriseSearchEngineDetails
but GET a single search application response indices type is String[]
would that be any issue in general ?
On side note we do update the hydrated indices in here
7eb14bf
to
ea21c98
Compare
`@elasticmachine merge upstream |
@elasticmachine merge upstream |
x-pack/plugins/enterprise_search/server/routes/enterprise_search/engines.ts
Outdated
Show resolved
Hide resolved
...earch/public/applications/enterprise_search_content/components/engines/engines_list_logic.ts
Show resolved
Hide resolved
5ba36eb
to
c383c5f
Compare
13f58ae
to
6100bc9
Compare
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
This PR adds a lib file which fetches the indices stats of an search application from Elasticsearch directly
Screenshot
List page indices
Overview page indices
Checklist