Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Index Patterns] Use deprecation api for scripted fields #100781
[Index Patterns] Use deprecation api for scripted fields #100781
Changes from 1 commit
c66111d
88f4db9
7339216
0737efb
767ee80
442121a
7b55f1e
8edffc1
a56b102
d3d1509
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@Bamieh do you think we should introduce some kind of caching in the deprecation service? atm everytime the
/api/deprecations
route is called, all the deprecation providers are executed, and I wonder if this will be alright when we'll have a lot that are performing ES requests like this one. wdyt?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.
If we do, not sure how it will work exactly? It is likely that with the cashing in place the use case I expressed in e2e testtest/api_integration/apis/index_patterns/deprecations/scripted_fields.ts might break?
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.
Also @pgayvallet, I think having so many index patterns is more of an edge case and inside the app we simply currently do
kibana/src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts
Lines 96 to 100 in 51616e1
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.
@Dosant this should not impact this PR, I was more wondering about long-term performances.
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.
Initially i had caching in place for the service but it didnt make sense to keep it. The API is called only at the user's request when they open the UA or explicitly via the API. Generic caching wont work since we do not want to return already resolved deprecations from the api. We always want the latest state of the deprecations.
For performance it might make sense to introduce pagination rather than caching. The service would paginate the kibana deprecations so we dont have to loop over all deprecations every time. Since the deprecations are registered at setup in an array we can guarantee the order and create basic pagination from the public contract.
I agree with @pgayvallet this doesnt impact the PR. I'll open a separate issue to continue the discussion there.
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 intentionally don't fetch all index patterns to make this function cheaper. This is reflected in the wording of the message below:
You have **at least** <n> index patterns...
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 should show a full list of index patterns that need updating as some setups may have a large number of index patterns and only a few might have scripted fields. We need a way to deliminate the index patterns as they can have commas in them. Could we link directly to the index patterns?
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.
Nope, as I understand we can only use plain text.
At this moment this pr shows at max 3 index pattern titles as a hint.
.slice(0,3)
where I prepare the messageIn the current implementation user can:
Not ideal, but not sure we want to fetch all the index patterns?
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 updated so in the main message we show just up to 3 index pattern titles, but then when you open dialog with details we list all of them!
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.
Might need some help with wording
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's going to be a blog post about this shortly, lets update this text once its available