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

[Discover] Show ignored field values #115040

Merged
merged 51 commits into from
Oct 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
9765492
WIP replacing indexPattern.flattenHit by tabify
Oct 11, 2021
76e44ae
Fix jest tests
Oct 12, 2021
4304f55
Read metaFields from index pattern
Oct 12, 2021
13bb111
Remove old test code
Oct 12, 2021
07c2231
remove unnecessary changes
Oct 12, 2021
983f830
Remove flattenHitWrapper APIs
Oct 12, 2021
8be2525
Fix imports
Oct 12, 2021
5c47379
Fix missing metaFields
Oct 12, 2021
8292e0c
Merge branch 'master' into dataview/flatten-hit-deprecation
kibanamachine Oct 12, 2021
6ebbf26
Add all meta fields to allowlist
Oct 12, 2021
921bd6c
Merge branch 'dataview/flatten-hit-deprecation' of github.com:timroes…
Oct 12, 2021
066c7aa
Merge branch 'master' into dataview/flatten-hit-deprecation
kibanamachine Oct 13, 2021
5349298
Improve inline comments
Oct 13, 2021
911cf9e
Move flattenHit test to new implementation
Oct 13, 2021
f890df2
Add deprecation comment to implementation
Oct 13, 2021
ce17147
Merge branch 'dataview/flatten-hit-deprecation' into ignored-field-va…
Oct 14, 2021
4f02850
WIP - Show ignored field values
Oct 14, 2021
afad073
Disable filters in doc_table
Oct 14, 2021
7829357
Merge branch 'master' into ignored-field-values
Oct 14, 2021
18e2205
remove redundant comments
Oct 14, 2021
71ac26c
No, it wasn't
Oct 14, 2021
3dc6746
start warning message
Oct 14, 2021
3dc160c
Enable ignored values in CSV reports
Oct 14, 2021
a746799
Add help tooltip
Oct 15, 2021
ddfaf53
Better styling with warning plus collapsible button
Oct 15, 2021
f164811
Disable filtering within table for ignored values
Oct 15, 2021
50799bd
Fix jest tests
Oct 15, 2021
cc07ef4
Fix types in tests
Oct 15, 2021
0337baf
Add more tests and documentation
Oct 15, 2021
1fd926c
Remove comment
Oct 15, 2021
95a31c5
Move dangerouslySetInnerHTML into helper method
Oct 16, 2021
17ed7fa
Extract document formatting into common utility
Oct 16, 2021
1d2f19f
Remove HTML source field formatter
Oct 16, 2021
6966db1
Move formatHit to Discover
Oct 16, 2021
22be128
Change wording of ignored warning
Oct 16, 2021
97c4984
Add cache for formatted hits
Oct 16, 2021
0e6ef91
Remove dead type
Oct 16, 2021
7713e5c
Fix row_formatter for objects
Oct 16, 2021
81f2b88
Improve mobile layout
Oct 16, 2021
9d5f174
Fix jest tests
Oct 16, 2021
57c0130
Fix typo
Oct 16, 2021
79abe37
Remove additional span again
Oct 16, 2021
1802dc7
Merge branch 'master' into ignored-field-values
kibanamachine Oct 18, 2021
e24d0dd
Change mock to revert test
Oct 18, 2021
5e3974a
Improve tests
Oct 18, 2021
358b77d
More jest tests
Oct 18, 2021
3863d3b
Fix typo
Oct 18, 2021
54b03ce
Change wording
Oct 19, 2021
88be299
Merge branch 'master' into ignored-field-values
kibanamachine Oct 19, 2021
267334d
Remove dead comment
Oct 19, 2021
0aa05b1
Merge branch 'ignored-field-values' of github.com:timroes/kibana into…
Oct 19, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/plugins/data/common/search/tabify/tabify_docs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ function create(id: string) {
});
}

const meta = {
_index: 'index-name',
_id: '1',
};

describe('tabify_docs', () => {
describe('flattenHit', () => {
let indexPattern: DataView;
Expand Down Expand Up @@ -70,6 +75,50 @@ describe('tabify_docs', () => {
expect(Object.keys(response)).toEqual(expectedOrder);
expect(Object.entries(response).map(([key]) => key)).toEqual(expectedOrder);
});

it('does merge values from ignored_field_values and fields correctly', () => {
const flatten = flattenHit(
{
...meta,
fields: { 'extension.keyword': ['foo'], extension: ['foo', 'ignored'] },
ignored_field_values: {
'extension.keyword': ['ignored'],
fully_ignored: ['some', 'value'],
},
},
indexPattern,
{ includeIgnoredValues: true }
);
expect(flatten).toHaveProperty(['extension.keyword'], ['foo', 'ignored']);
expect(flatten).toHaveProperty('extension', ['foo', 'ignored']);
expect(flatten).toHaveProperty('fully_ignored', ['some', 'value']);
});

it('does not merge values from ignored_field_values into _source', () => {
const flatten = flattenHit(
{
...meta,
_source: { 'extension.keyword': ['foo', 'ignored'] },
ignored_field_values: { 'extension.keyword': ['ignored'] },
},
indexPattern,
{ includeIgnoredValues: true, source: true }
);
expect(flatten).toHaveProperty(['extension.keyword'], ['foo', 'ignored']);
});

it('does merge ignored_field_values when no _source was present, even when parameter was on', () => {
const flatten = flattenHit(
{
...meta,
fields: { 'extension.keyword': ['foo'] },
ignored_field_values: { 'extension.keyword': ['ignored'] },
},
indexPattern,
{ includeIgnoredValues: true, source: true }
);
expect(flatten).toHaveProperty(['extension.keyword'], ['foo', 'ignored']);
});
});

describe('tabifyDocs', () => {
Expand Down
38 changes: 33 additions & 5 deletions src/plugins/data/common/search/tabify/tabify_docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,18 @@ export interface TabifyDocsOptions {
* merged into the flattened document.
*/
source?: boolean;
/**
* If set to `true` values that have been ignored in ES (ignored_field_values)
* will be merged into the flattened document. This will only have an effect if
* the `hit` has been retrieved using the `fields` option.
*/
includeIgnoredValues?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason we don't make this default to true?

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've talked a bit in the PR description about this. I believe that the behavior of merging a response from the fields API with ignored values together might be more of an edge-case that we need in Discover (where users are more interested "in the document" than in the fields), thus I don't think we should have that on by default. Those values coming from that ignored API might cause other weird side-effects potentially, so I think we should make merging them into the flattened object and opt-in imho.

}

// This is an overwrite of the SearchHit type to add the ignored_field_values.
// Can be removed once the estypes.SearchHit knows about ignored_field_values
type Hit<T = unknown> = estypes.SearchHit<T> & { ignored_field_values?: Record<string, unknown[]> };

/**
* Flattens an individual hit (from an ES response) into an object. This will
* create flattened field names, like `user.name`.
Expand All @@ -65,11 +75,7 @@ export interface TabifyDocsOptions {
* @param indexPattern The index pattern for the requested index if available.
* @param params Parameters how to flatten the hit
*/
export function flattenHit(
hit: estypes.SearchHit,
indexPattern?: IndexPattern,
params?: TabifyDocsOptions
) {
export function flattenHit(hit: Hit, indexPattern?: IndexPattern, params?: TabifyDocsOptions) {
const flat = {} as Record<string, any>;

function flatten(obj: Record<string, any>, keyPrefix: string = '') {
Expand Down Expand Up @@ -109,6 +115,28 @@ export function flattenHit(
flatten(hit.fields || {});
if (params?.source !== false && hit._source) {
flatten(hit._source as Record<string, any>);
} else if (params?.includeIgnoredValues && hit.ignored_field_values) {
// If enabled merge the ignored_field_values into the flattened hit. This will
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I am missing something, but shouldn't there be a check if fields are being read from fields API then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignored_field_values will only be set if requesting the fields API, and thus we're treating this here the same as the rest of that flatten logic: we ignore the actual request and purely look at the response to flatten, e.g. we also merge fields above ignoring what the request might have been, and simply try to merge it if it's available.

The only case here (as stated by the comment) is, that there might be theoretically _source and fields and ignored_field_values, in which case we don't want to merge the ignored_field_values, since all those values were already in _source and thus we're only doing this logic if we didn't merge _source above.

// merge values that are not actually indexed by ES (i.e. ignored), e.g. because
// they were above the `ignore_above` limit or malformed for specific types.
// This API will only contain the values that were actually ignored, i.e. for the same
// field there might exist another value in the `fields` response, why this logic
// merged them both together. We do not merge this (even if enabled) in case source has been
// merged, since we would otherwise duplicate values, since ignore_field_values and _source
// contain the same values.
Object.entries(hit.ignored_field_values).forEach(([fieldName, fieldValue]) => {
if (flat[fieldName]) {
// If there was already a value from the fields API, make sure we're merging both together
if (Array.isArray(flat[fieldName])) {
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 think this would be more readable if it was all one if-statement:

const valueExists = !!flat[fieldName]
if (valueExists && Array.isArray(flat[fieldName])) 
....
else if (valueExists)
...
else
...

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 have mixed feelings around that. I feel making the 2nd else basically dependant on the first (in the sense, that the first one is not hit and then recheck half of the condition from the first), imho does not increase readability. We also don't do this in other places, e.g. https://github.com/timroes/kibana/blob/ignored-field-values/src/plugins/discover/public/application/helpers/format_hit.ts#L55-L61 which could also be transformed the same way into:

if (displayKey && fieldsToShow.includes(key)) {
  pairs.push([displayKey, formattedValue]);
} else if (!displayKey) {
  pairs.push([key, formattedValue]);
}

So maybe we can leave that to a democratic vote :)

🚀 - Flatten this out into one dimension as suggested above
🎉 - Keep it in the current nested way

cc @kertal @dmitriynj

Copy link
Contributor

Choose a reason for hiding this comment

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

"I don't think it would be more readable the way you suggest, Maja, I'd like to keep it as it is" would have been a perfectly fine response

flat[fieldName] = [...flat[fieldName], ...fieldValue];
} else {
flat[fieldName] = [flat[fieldName], ...fieldValue];
}
} else {
// If no previous value was assigned we can simply use the value from `ignored_field_values` as it is
flat[fieldName] = fieldValue;
}
});
}

// Merge all valid meta fields into the flattened object
Expand Down
13 changes: 0 additions & 13 deletions src/plugins/data_views/common/data_views/data_view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { DuplicateField } from '../../../kibana_utils/common';

import { IIndexPattern, IFieldType } from '../../common';
import { DataViewField, IIndexPatternFieldList, fieldList } from '../fields';
import { formatHitProvider } from './format_hit';
import { flattenHitWrapper } from './flatten_hit';
import {
FieldFormatsStartCommon,
Expand Down Expand Up @@ -45,8 +44,6 @@ interface SavedObjectBody {
type?: string;
}

type FormatFieldFn = (hit: Record<string, any>, fieldName: string) => any;

export class DataView implements IIndexPattern {
public id?: string;
public title: string = '';
Expand All @@ -67,11 +64,6 @@ export class DataView implements IIndexPattern {
* Type is used to identify rollup index patterns
*/
public type: string | undefined;
public formatHit: {
(hit: Record<string, any>, type?: string): any;
formatField: FormatFieldFn;
};
public formatField: FormatFieldFn;
/**
* @deprecated Use `flattenHit` utility method exported from data plugin instead.
*/
Expand Down Expand Up @@ -103,11 +95,6 @@ export class DataView implements IIndexPattern {
this.fields = fieldList([], this.shortDotsEnable);

this.flattenHit = flattenHitWrapper(this, metaFields);
this.formatHit = formatHitProvider(
this,
fieldFormats.getDefaultInstance(KBN_FIELD_TYPES.STRING)
);
this.formatField = this.formatHit.formatField;

// set values
this.id = spec.id;
Expand Down
74 changes: 0 additions & 74 deletions src/plugins/data_views/common/data_views/format_hit.ts

This file was deleted.

1 change: 0 additions & 1 deletion src/plugins/data_views/common/data_views/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,5 @@

export * from './_pattern_cache';
export * from './flatten_hit';
export * from './format_hit';
export * from './data_view';
export * from './data_views';
2 changes: 1 addition & 1 deletion src/plugins/data_views/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export {
ILLEGAL_CHARACTERS,
validateDataView,
} from '../common/lib';
export { formatHitProvider, onRedirectNoIndexPattern } from './data_views';
export { onRedirectNoIndexPattern } from './data_views';

export { IndexPatternField, IIndexPatternFieldList, TypeMeta } from '../common';

Expand Down
3 changes: 2 additions & 1 deletion src/plugins/discover/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"data",
"embeddable",
"inspector",
"fieldFormats",
"kibanaLegacy",
"urlForwarding",
"navigation",
Expand All @@ -16,7 +17,7 @@
"indexPatternFieldEditor"
],
"optionalPlugins": ["home", "share", "usageCollection", "spaces"],
"requiredBundles": ["kibanaUtils", "home", "kibanaReact", "fieldFormats", "dataViews"],
"requiredBundles": ["kibanaUtils", "home", "kibanaReact", "dataViews"],
"extraPublicDirs": ["common"],
"owner": {
"name": "Data Discovery",
Expand Down
18 changes: 7 additions & 11 deletions src/plugins/discover/public/__mocks__/index_pattern.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@
* Side Public License, v 1.
*/

import type { estypes } from '@elastic/elasticsearch';
import { flattenHit, IIndexPatternFieldList } from '../../../data/common';
import { IIndexPatternFieldList } from '../../../data/common';
import { IndexPattern } from '../../../data/common';

const fields = [
Expand All @@ -28,33 +27,38 @@ const fields = [
{
name: 'message',
type: 'string',
displayName: 'message',
scripted: false,
filterable: false,
aggregatable: false,
},
{
name: 'extension',
type: 'string',
displayName: 'extension',
scripted: false,
filterable: true,
aggregatable: true,
},
{
name: 'bytes',
type: 'number',
displayName: 'bytesDisplayName',
scripted: false,
filterable: true,
aggregatable: true,
},
{
name: 'scripted',
type: 'number',
displayName: 'scripted',
scripted: true,
filterable: false,
},
{
name: 'object.value',
type: 'number',
displayName: 'object.value',
scripted: false,
filterable: true,
aggregatable: true,
Expand All @@ -73,23 +77,15 @@ const indexPattern = {
id: 'the-index-pattern-id',
title: 'the-index-pattern-title',
metaFields: ['_index', '_score'],
formatField: jest.fn(),
flattenHit: undefined,
formatHit: jest.fn((hit) => (hit.fields ? hit.fields : hit._source)),
fields,
getComputedFields: () => ({ docvalueFields: [], scriptFields: {}, storedFields: ['*'] }),
getSourceFiltering: () => ({}),
getFieldByName: jest.fn(() => ({})),
timeFieldName: '',
docvalueFields: [],
getFormatterForField: () => ({ convert: () => 'formatted' }),
getFormatterForField: jest.fn(() => ({ convert: (value: unknown) => value })),
} as unknown as IndexPattern;

indexPattern.isTimeBased = () => !!indexPattern.timeFieldName;
indexPattern.formatField = (hit: Record<string, unknown>, fieldName: string) => {
return fieldName === '_source'
? hit._source
: flattenHit(hit as unknown as estypes.SearchHit, indexPattern)[fieldName];
};

export const indexPatternMock = indexPattern;
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@
* Side Public License, v 1.
*/

import { flattenHit, IIndexPatternFieldList } from '../../../data/common';
import { IIndexPatternFieldList } from '../../../data/common';
import { IndexPattern } from '../../../data/common';
import type { estypes } from '@elastic/elasticsearch';

const fields = [
{
Expand Down Expand Up @@ -64,23 +63,16 @@ const indexPattern = {
id: 'index-pattern-with-timefield-id',
title: 'index-pattern-with-timefield',
metaFields: ['_index', '_score'],
flattenHit: undefined,
formatHit: jest.fn((hit) => hit._source),
fields,
getComputedFields: () => ({}),
getSourceFiltering: () => ({}),
getFieldByName: (name: string) => fields.getByName(name),
timeFieldName: 'timestamp',
getFormatterForField: () => ({ convert: () => 'formatted' }),
getFormatterForField: () => ({ convert: (value: unknown) => value }),
isTimeNanosBased: () => false,
popularizeField: () => {},
} as unknown as IndexPattern;

indexPattern.isTimeBased = () => !!indexPattern.timeFieldName;
indexPattern.formatField = (hit: Record<string, unknown>, fieldName: string) => {
return fieldName === '_source'
? hit._source
: flattenHit(hit as unknown as estypes.SearchHit, indexPattern)[fieldName];
};

export const indexPatternWithTimefieldMock = indexPattern;
Loading