Skip to content
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

Merged
9 changes: 9 additions & 0 deletions src/plugins/data/server/index_patterns/deprications/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
Dosant marked this conversation as resolved.
Show resolved Hide resolved
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

export { createScriptedFieldsDeprecationsConfig } from './scripted_fields';
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { hasScriptedField } from './scripted_fields';

describe('hasScriptedField', () => {
test('valid index pattern object with a scripted field', () => {
expect(
hasScriptedField({
title: 'kibana_sample_data_logs*',
fields:
'[{"count":0,"script":"return 5;","lang":"painless","name":"test","type":"number","scripted":true,"searchable":true,"aggregatable":true,"readFromDocValues":false,"customLabel":""}]',
})
).toBe(true);
});

test('valid index pattern object without a scripted field', () => {
expect(
hasScriptedField({
title: 'kibana_sample_data_logs*',
fields: '[]',
})
).toBe(false);
});

test('invalid index pattern object', () => {
expect(
hasScriptedField({
title: 'kibana_sample_data_logs*',
fields: '[...]',
})
).toBe(false);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import {
CoreSetup,
DeprecationsDetails,
GetDeprecationsContext,
RegisterDeprecationsConfig,
} from 'kibana/server';
import { IndexPatternAttributes } from '../../../common';

type IndexPatternAttributesWithFields = Pick<IndexPatternAttributes, 'title' | 'fields'>;

export const createScriptedFieldsDeprecationsConfig: (
core: CoreSetup
) => RegisterDeprecationsConfig = (core: CoreSetup) => ({
getDeprecations: async (context: GetDeprecationsContext): Promise<DeprecationsDetails[]> => {
const finder = context.savedObjectsClient.createPointInTimeFinder<IndexPatternAttributesWithFields>(
{
type: 'index-pattern',
perPage: 100,
fields: ['title', 'fields'],
}
);
Copy link
Contributor

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?

Copy link
Contributor Author

@Dosant Dosant May 28, 2021

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?

Copy link
Contributor Author

@Dosant Dosant May 28, 2021

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

const so = await this.savedObjectsClient.find<IndexPatternSavedObjectAttrs>({
type: 'index-pattern',
fields: ['title'],
perPage: 10000,
});

Copy link
Contributor

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.

Copy link
Member

@Bamieh Bamieh Jun 1, 2021

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.


let indexPatternsWithScriptedFields: IndexPatternAttributesWithFields[] = [];
for await (const response of finder.find()) {
Dosant marked this conversation as resolved.
Show resolved Hide resolved
indexPatternsWithScriptedFields = response.saved_objects
.map((so) => so.attributes)
.filter(hasScriptedField);

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;
Copy link
Contributor Author

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...

Copy link
Contributor

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?

Copy link
Contributor Author

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.

  1. 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
  2. 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:

  1. Take a look at a message, see up to 3 index pattern titles that need to be fixed
  2. Fix them
  3. 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?

Copy link
Contributor Author

@Dosant Dosant May 28, 2021

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!

}
}

if (indexPatternsWithScriptedFields.length > 0) {
const indexPatternsTitlesHelp = indexPatternsWithScriptedFields
.map((ip) => ip.title)
.slice(0, 3) // to avoid very long message
.join(', ');

return [
{
message: `You have at least ${indexPatternsWithScriptedFields.length} index patterns (${indexPatternsTitlesHelp}...) that use scripted fields. Scripted fields are deprecated and will be removed in future. Use runtime fields instead.`,
documentationUrl:
'https://www.elastic.co/guide/en/elasticsearch/reference/7.x/runtime.html', // TODO: documentation service is not available serverside https://github.com/elastic/kibana/issues/95389
level: 'warning', // warning because it is not set in stone WHEN we remove scripted fields, hence this deprecation is not a blocker for 8.0 upgrade
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.`,
Copy link
Contributor Author

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

Copy link
Contributor

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

],
},
},
];
} else {
return [];
}
},
});

export function hasScriptedField(indexPattern: IndexPatternAttributesWithFields) {
if (indexPattern.fields) {
try {
return JSON.parse(indexPattern.fields).some(
(field: { scripted?: boolean }) => field?.scripted
);
} catch (e) {
return false;
}
} else {
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { UiSettingsServerToCommon } from './ui_settings_wrapper';
import { IndexPatternsApiServer } from './index_patterns_api_client';
import { SavedObjectsClientServerToCommon } from './saved_objects_client_wrapper';
import { registerIndexPatternsUsageCollector } from './register_index_pattern_usage_collection';
import { createScriptedFieldsDeprecationsConfig } from './deprications';

export interface IndexPatternsServiceStart {
indexPatternsServiceFactory: (
Expand Down Expand Up @@ -88,6 +89,7 @@ export class IndexPatternsServiceProvider implements Plugin<void, IndexPatternsS

expressions.registerFunction(getIndexPatternLoad({ getStartServices: core.getStartServices }));
registerIndexPatternsUsageCollector(core.getStartServices, usageCollection);
core.deprecations.registerDeprecations(createScriptedFieldsDeprecationsConfig(core));
}

public start(core: CoreStart, { fieldFormats, logger }: IndexPatternsServiceStartDeps) {
Expand Down
15 changes: 15 additions & 0 deletions test/api_integration/apis/index_patterns/deprecations/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { FtrProviderContext } from '../../../ftr_provider_context';

export default function ({ loadTestFile }: FtrProviderContext) {
describe('scripted_fields_deprecations', () => {
loadTestFile(require.resolve('./scripted_fields'));
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import expect from '@kbn/expect';
import type { DeprecationsGetResponse } from 'src/core/server/types';
import { FtrProviderContext } from '../../../ftr_provider_context';

export default function ({ getService }: FtrProviderContext) {
const supertest = getService('supertest');
const esArchiver = getService('esArchiver');

describe('scripted field deprecations', () => {
before(async () => {
await esArchiver.emptyKibanaIndex();
await esArchiver.load('index_patterns/basic_index');
});

after(async () => {
await esArchiver.unload('index_patterns/basic_index');
});

it('no scripted fields deprecations', async () => {
const { body } = await supertest.get('/api/deprecations/');

const { deprecations } = body as DeprecationsGetResponse;
const dataPluginDeprecations = deprecations.filter(({ domainId }) => domainId === 'data');

expect(dataPluginDeprecations.length).to.be(0);
});

it('scripted field deprecation', async () => {
const title = `basic_index`;
await supertest.post('/api/index_patterns/index_pattern').send({
index_pattern: {
title,
fields: {
foo: {
name: 'foo',
type: 'string',
scripted: true,
script: "doc['field_name'].value",
},
bar: {
name: 'bar',
type: 'number',
scripted: true,
script: "doc['field_name'].value",
},
},
},
});

const { body } = await supertest.get('/api/deprecations/');
const { deprecations } = body as DeprecationsGetResponse;
const dataPluginDeprecations = deprecations.filter(({ domainId }) => domainId === 'data');

expect(dataPluginDeprecations.length).to.be(1);
expect(dataPluginDeprecations[0].message).to.contain(title);
});
});
}
1 change: 1 addition & 0 deletions test/api_integration/apis/index_patterns/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ export default function ({ loadTestFile }) {
loadTestFile(require.resolve('./index_pattern_crud'));
loadTestFile(require.resolve('./scripted_fields_crud'));
loadTestFile(require.resolve('./fields_api'));
loadTestFile(require.resolve('./deprecations'));
});
}