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] [skip-ci] Index pattern “pattern list” support #89330

Closed
wants to merge 45 commits into from

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Jan 26, 2021

WIP

This is a Work In Progress PR. I'm sorry it is going to be quite large as this is a breaking change to Kibana index patterns.

Resolves the first part of #87851

  • Restructure index-pattern saved object to accommodate patternList
    • title remains as a label for the index pattern
    • title will be migrated to patternList from a string to a string array, separated by commas.
      • ex: { title: "filebeat-*,auditbeat-*" } becomes { title: "filebeat-*,auditbeat-*", patternList: ["filebeat-*","auditbeat-*"] }
    • going forward, title will be a descriptive field
      • ex: { title: "Security Solution Indices", patternList: ["filebeat-*","auditbeat-*"] }
  • at runtime, detects index patterns in the patternList that do not match data and returns patternListActive

Plugin Changes (so far, WIP y'all)

  • Discover: updates sidebar index pattern selector UI
  • TSVB: update fetchFields function that checked title to check patternListActive instead

Checklist

Delete any items that are not applicable to this PR.

For maintainers

(rif) => rif !== false
) as FieldDescriptor[][];
const fields = !formatFields
? await combineFields(responsesIndexFields)
Copy link
Contributor Author

@stephmilovic stephmilovic Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XavierM this is the documenter boolean. should i include this and the formatIndexFields?

intervalName,
fields,
sourceFilters,
allowNoIndex,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the extra line changes. It is so much easier to see where updates have been made to objects/types when their properties are alphabetized, especially with bigger objects. I did this in a few places throughout this PR

@@ -96,20 +96,15 @@ export default new Datasource('es', {
kibana: true,
fit: 'nearest',
});

const findResp = await tlConfig.savedObjectsClient.find({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the patternListActive is generated at runtime, it is not stored on the index-pattern saved object. I changed this to fetch the index patterns from the indexPatternsService.getPatternCache(), which will give you a list of index patterns including the runtime patternListActive property

@@ -79,6 +79,16 @@ export function runRoute(
try {
const uiSettings = await context.core.uiSettings.client.getAll();

const getIndexPatternsService = async () => {
// @ts-ignore need help resolving this one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to fix that, please update type in line 36 CoreSetup -> CoreSetup<TimelionPluginStartDeps>

image

@@ -88,6 +98,7 @@ export function runRoute(
allowedGraphiteUrls: configManager.getGraphiteUrls(),
esShardTimeout: configManager.getEsShardTimeout(),
savedObjectsClient: context.core.savedObjects.client,
getIndexPatternsService: async () => indexPatternsService,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I see that code was copied from TSVB. But it that plugin we had a reason to return indexPatternsService in a Promise. For your case it doesn't have a lot of sense.

I suggest you to remove line 91: const indexPatternsService = await getIndexPatternsService();

And here write:

{
    savedObjectsClient: context.core.savedObjects.client,
    getIndexPatternsService,
}

@@ -62,7 +62,7 @@ export async function getFields(
if (!indexPatternString) {
const defaultIndexPattern = await indexPatternsService.getDefault();

indexPatternString = defaultIndexPattern?.title ?? '';
indexPatternString = defaultIndexPattern?.patternListActive.join(',') ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan to do patternListActive.join(',') on each places where we need to get string representation of index pattern. From my perspective it should be encapsulated as a method in IndexPattern service: e.g getIndexPatternString() or just a toString()

);
}
return patterns.sort((a, b) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: what about sortBy(patterns, 'sort') from lodash?

if (containsIllegalCharacters(searchValue, indexPatterns.ILLEGAL_CHARACTERS)) {
return false;
}
onQueryChanged([...selectedPatterns, searchValue]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure that this code works correctly

if I input index-a,index-b,index-c. It should be as least splitted by comma and added as a three separate indexes.

Screen.Recording.2021-01-29.at.1.43.02.PM.mov

to discuss: some time ago we tried to solve the same problem on TSVB plugin. From my point of view here instead of EuiComboBox we should use EuiSuggest component. See #82696

const indexPatternSavedObject = findResp.saved_objects.find((savedObject) => {
return savedObject.attributes.title === config.index;
const indexPatternsService = await tlConfig.getIndexPatternsService();
const searchIndexPatterns = await indexPatternsService.find(config.index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not working as expected. To reproduce that, let's create the following index:
image

and then let's try to find that index using indexPatternsService.find method. I used the following snippet:

console.log('f1', await indexPatterns.find('f1'));
console.log('index-a,index-b,index-c,index-d', await indexPatterns.find('index-a,index-b,index-c,index-d'));

Result of that execution looks like:
image

it means that in that method we are looking by title not by patternList

@@ -11,6 +11,7 @@ import { dataTypes, installationStatuses } from '../../../../../common/constants
import { ArchivePackage, Installation, InstallSource, ValueOf } from '../../../../../common/types';
import { RegistryPackage, DataType } from '../../../../types';
import { getInstallation, getPackageFromSource, getPackageSavedObjects } from '../../packages/get';
import { IndexPatternAttributes } from '../../../../../../../../src/plugins/data/common/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.

nit: could you please fix import. '../../../../../../../../src/plugins/data/server'

return savedObject.attributes.title === config.index;
const indexPatternsService = await tlConfig.getIndexPatternsService();
const searchIndexPatterns = await indexPatternsService.find(config.index);
const indexPatternSpec = searchIndexPatterns.find((indexPattern) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timroes not sure that I fully understand how it should be. Before we could not create two indexes with the same title. Now, title it's just a name and for mapping ES indexes we use patternList property. It means that I can do something like:
image

Let's complicate the case a bit and make changes to the f2 index. You can set a custom field or add a new scripted field. It does not matter. For my example I added a scripted field

image

now let's go back to that code. So: searchIndexPatterns - it's an array which keeps 2 indexes (f1, f2). 'find' by array returns the first matched result. It's f1.

But what if I need to work with f2?

I really like that code and I think that it's a good enchantment for our users. But have no idea how TSVB, Timelion and Vega should work with that.

@ryankeairns
Copy link
Contributor

This bit of UI is repeated in other places (e.g. Lens), so we ought to get their take once you're ready for wider feedback. Thanks for tackling this change.

@stephmilovic
Copy link
Contributor Author

Closing until a few prerequisite updates are taken care of: #90076

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants