-
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
[Index Patterns] Use deprecation api for scripted fields #100781
[Index Patterns] Use deprecation api for scripted fields #100781
Conversation
src/plugins/data/server/index_patterns/deprications/scripted_fields.ts
Outdated
Show resolved
Hide resolved
if (indexPatternsWithScriptedFields.length > 0) { | ||
// no need to look up all index patterns since we've found at least one that has scripted fields | ||
await finder.close(); | ||
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.
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.
Could we link directly to the index patterns?
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.
- Do you think we should show more out from the page? (e.g. in the current implementation we can remove that
.slice(0,3)
where I prepare the message - Or do you think we should fetch all the index patterns and show the whole list no matter how long it is?
In the current implementation user can:
- Take a look at a message, see up to 3 index pattern titles that need to be fixed
- Fix them
- If there are more to fix, see another 3 index pattern titles and fix them
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!
correctiveActions: { | ||
manualSteps: [ | ||
'Navigate to Stack Management > Kibana > Index Patterns.', | ||
`Update index patterns that have scripted fields (${indexPatternsTitlesHelp}...) to use runtime fields instead. In most cases, to migrate existing scripts, changing "return <value>;" to "emit(<value>);" would be enough.`, |
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
Pinging @elastic/kibana-app-services (Team:AppServices) |
correctiveActions: { | ||
manualSteps: [ | ||
'Navigate to Stack Management > Kibana > Index Patterns.', | ||
`Update index patterns that have scripted fields (${indexPatternsTitlesHelp}...) to use runtime fields instead. In most cases, to migrate existing scripts, changing "return <value>;" to "emit(<value>);" would be enough.`, |
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
if (indexPatternsWithScriptedFields.length > 0) { | ||
// no need to look up all index patterns since we've found at least one that has scripted fields | ||
await finder.close(); | ||
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.
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?
|
const finder = context.savedObjectsClient.createPointInTimeFinder<IndexPatternAttributesWithFields>( | ||
{ | ||
type: 'index-pattern', | ||
perPage: 100, | ||
fields: ['title', 'fields'], | ||
} | ||
); |
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
const so = await this.savedObjectsClient.find<IndexPatternSavedObjectAttrs>({ | |
type: 'index-pattern', | |
fields: ['title'], | |
perPage: 10000, | |
}); |
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.
I think we can't change this atm |
…cation-api-for-index-pattern # Conflicts: # test/api_integration/apis/index_patterns/index.js
I'm talking to @alisonelizabeth to see how this could be tested by QA. The UI for the deprecation service might not be available until 7.15. Considering how to proceed. Looks good and works well, waiting to hear from Alison about how we want this to go through QA. |
@mattkime we have this issue to improve this #99625
I'm trying to address this as well (context: #99043 (comment)) The service provided by core is testable although we do need UA to provide some tools to allow teams to run functional tests for their deprecations in the UA UI |
@elasticmachine merge upstream |
…cation-api-for-index-pattern # Conflicts: # test/api_integration/apis/index_patterns/index.js
…m:Dosant/kibana into dev/use-deprecation-api-for-index-pattern
@elasticmachine merge upstream |
@Dosant Do you know where we are on getting this merged? I know we were waiting for a flag to be able to test this without recompiling. |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…104654) Co-authored-by: Anton Dosov <[email protected]>
Summary
Close #97947
Scripted fields are deprecated. Runtime fields should be used instead. This pr adds information about this to the upgrade assistant. It hasn't been decided when scripted fields will be removed yet, hence keeping this as
warning
.Release notes
Add scripted fields depreciation information to upgrade assistant
Checklist
For maintainers
How to test
I viewed upgrade assistant UI by tweaking the condition in the code
kibana/x-pack/plugins/upgrade_assistant/public/application/app.tsx
Lines 26 to 28 in 186b936