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 20 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
29 changes: 24 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,17 @@ 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.
*/
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 +74,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 @@ -111,6 +116,20 @@ export function flattenHit(
flatten(hit._source as Record<string, any>);
}

if (params?.includeIgnoredValues && hit.ignored_field_values) {
timroes marked this conversation as resolved.
Show resolved Hide resolved
Object.entries(hit.ignored_field_values).forEach(([fieldName, fieldValue]) => {
if (flat[fieldName]) {
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 {
flat[fieldName] = fieldValue;
}
});
}

// Merge all valid meta fields into the flattened object
// expect for _source (in case that was specified as a meta field)
indexPattern?.metaFields?.forEach((metaFieldName) => {
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
8 changes: 1 addition & 7 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 Down Expand Up @@ -86,10 +85,5 @@ const indexPattern = {
} 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 @@ -77,10 +76,5 @@ const indexPattern = {
} 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;
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import React, { Fragment, useCallback, useMemo, useState } from 'react';
import classNames from 'classnames';
import { i18n } from '@kbn/i18n';
import { EuiButtonEmpty, EuiIcon } from '@elastic/eui';
import { formatFieldValue } from '../../../../../helpers/format_value';
import { flattenHit } from '../../../../../../../../data/common';
import { DocViewer } from '../../../../../components/doc_viewer/doc_viewer';
import { FilterManager, IndexPattern } from '../../../../../../../../data/public';
Expand Down Expand Up @@ -58,7 +59,10 @@ export const TableRow = ({
});
const anchorDocTableRowSubj = row.isAnchor ? ' docTableAnchorRow' : '';

const flattenedRow = useMemo(() => flattenHit(row, indexPattern), [indexPattern, row]);
const flattenedRow = useMemo(
() => flattenHit(row, indexPattern, { includeIgnoredValues: true }),
[indexPattern, row]
);
const mapping = useMemo(() => indexPattern.fields.getByName, [indexPattern]);

// toggle display of the rows details, a full list of the fields from each row
Expand All @@ -68,7 +72,12 @@ export const TableRow = ({
* Fill an element with the value of a field
*/
const displayField = (fieldName: string) => {
const formattedField = indexPattern.formatField(row, fieldName);
const formattedField = formatFieldValue(
flattenedRow[fieldName],
row,
indexPattern,
mapping(fieldName)
);

// field formatters take care of escaping
// eslint-disable-next-line react/no-danger
Expand Down Expand Up @@ -142,9 +151,9 @@ export const TableRow = ({
} else {
columns.forEach(function (column: string) {
// when useNewFieldsApi is true, addressing to the fields property is safe
if (useNewFieldsApi && !mapping(column) && !row.fields![column]) {
if (useNewFieldsApi && !mapping(column) && row.fields && !row.fields[column]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ This fixes another bug. row.fields could actually be undefined even when using fields API, in case all values in the document were actually ignored. On master Discover would simply crash if a document did not contain any fields but we used the new fields API. This is now solved.

Copy link
Contributor

@majagrubic majagrubic Oct 19, 2021

Choose a reason for hiding this comment

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

afaik, fields API should never not return at least an empty object. maybe worth checking with the ES team to see if this scenario is intentional?

const innerColumns = Object.fromEntries(
Object.entries(row.fields!).filter(([key]) => {
Object.entries(row.fields).filter(([key]) => {
return key.indexOf(`${column}.`) === 0;
})
);
Expand All @@ -161,7 +170,13 @@ export const TableRow = ({
/>
);
} else {
const isFilterable = Boolean(mapping(column)?.filterable && filter);
// Check whether the field is defined as filterable in the mapping and does
// NOT have ignored values in it to determine whether we want to allow filtering.
// We should improve this and show a helpful tooltip why the filter buttons are not
// there/disabled when there are ignored values.
const isFilterable = Boolean(
mapping(column)?.filterable && filter && !row._ignored?.includes(column)
);
rowCells.push(
<TableCell
key={column}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { flattenHit } from '../../../../../../../../data/common';
function getFieldValues(hits, field, indexPattern) {
const name = field.name;
return map(hits, function (hit) {
return flattenHit(hit, indexPattern)[name];
return flattenHit(hit, indexPattern, { includeIgnoredValues: true })[name];
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function calcFieldCounts(
return {};
}
for (const hit of rows) {
const fields = Object.keys(flattenHit(hit, indexPattern));
const fields = Object.keys(flattenHit(hit, indexPattern, { includeIgnoredValues: true }));
for (const fieldName of fields) {
counts[fieldName] = (counts[fieldName] || 0) + 1;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,11 @@ export const DiscoverGrid = ({
getRenderCellValueFn(
indexPattern,
displayedRows,
displayedRows ? displayedRows.map((hit) => flattenHit(hit, indexPattern)) : [],
displayedRows
? displayedRows.map((hit) =>
flattenHit(hit, indexPattern, { includeIgnoredValues: true })
)
: [],
useNewFieldsApi,
fieldsToShow,
services.uiSettings.get(MAX_DOC_FIELDS_DISPLAYED)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { DiscoverGridContext } from './discover_grid_context';
import { JsonCodeEditor } from '../json_code_editor/json_code_editor';
import { defaultMonacoEditorWidth } from './constants';
import { EsHitRecord } from '../../types';
import { formatFieldValue } from '../../helpers/format_value';

export const getRenderCellValueFn =
(
Expand Down Expand Up @@ -191,12 +192,12 @@ export const getRenderCellValueFn =
return <span>{JSON.stringify(rowFlattened[columnId])}</span>;
}

const valueFormatted = indexPattern.formatField(row, columnId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ A field formatter would always return a string, thus the next check was unnecessary and could never trigger, so I removed it.

const valueFormatted = formatFieldValue(rowFlattened[columnId], row, indexPattern, field);
if (typeof valueFormatted === 'undefined') {
return <span>-</span>;
}
return (
// eslint-disable-next-line react/no-danger
<span dangerouslySetInnerHTML={{ __html: indexPattern.formatField(row, columnId) }} />
<span dangerouslySetInnerHTML={{ __html: valueFormatted }} />
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import {
} from '../../doc_views/doc_views_types';
import { ACTIONS_COLUMN, MAIN_COLUMNS } from './table_columns';
import { getFieldsToShow } from '../../helpers/get_fields_to_show';
import { getIgnoredReason, IgnoredReason } from '../../helpers/get_ignored_reason';
import { formatFieldValue } from '../../helpers/format_value';

export interface DocViewerTableProps {
columns?: string[];
Expand All @@ -46,6 +48,7 @@ export interface FieldRecord {
};
value: {
formattedValue: string;
ignored?: IgnoredReason;
};
}

Expand All @@ -64,8 +67,6 @@ export const DocViewerTable = ({
[indexPattern?.fields]
);

const formattedHit = useMemo(() => indexPattern?.formatHit(hit, 'html'), [hit, indexPattern]);

const tableColumns = useMemo(() => {
return filter ? [ACTIONS_COLUMN, ...MAIN_COLUMNS] : MAIN_COLUMNS;
}, [filter]);
Expand Down Expand Up @@ -96,7 +97,7 @@ export const DocViewerTable = ({
return null;
}

const flattened = flattenHit(hit, indexPattern, { source: true });
const flattened = flattenHit(hit, indexPattern, { source: true, includeIgnoredValues: true });
const fieldsToShow = getFieldsToShow(Object.keys(flattened), indexPattern, showMultiFields);

const items: FieldRecord[] = Object.keys(flattened)
Expand All @@ -115,6 +116,8 @@ export const DocViewerTable = ({
const displayName = fieldMapping?.displayName ?? field;
const fieldType = isNestedFieldParent(field, indexPattern) ? 'nested' : fieldMapping?.type;

const ignored = getIgnoredReason(fieldMapping ?? field, hit._ignored);

return {
action: {
onToggleColumn,
Expand All @@ -130,7 +133,8 @@ export const DocViewerTable = ({
scripted: Boolean(fieldMapping?.scripted),
},
value: {
formattedValue: formattedHit[field],
formattedValue: formatFieldValue(flattened[field], hit, indexPattern, fieldMapping),
ignored,
},
};
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,24 @@

import classNames from 'classnames';
import React, { Fragment, useState } from 'react';
import { IgnoredReason } from '../../helpers/get_ignored_reason';
import { FieldRecord } from './table';
import { DocViewTableRowBtnCollapse } from './table_row_btn_collapse';

const COLLAPSE_LINE_LENGTH = 350;

type TableFieldValueProps = FieldRecord['value'] & Pick<FieldRecord['field'], 'field'>;
type TableFieldValueProps = Pick<FieldRecord['field'], 'field'> & {
formattedValue: FieldRecord['value']['formattedValue'];
rawValue: unknown;
ignoreReason?: IgnoredReason;
};

export const TableFieldValue = ({ formattedValue, field }: TableFieldValueProps) => {
export const TableFieldValue = ({
formattedValue,
field,
rawValue,
ignoreReason,
}: TableFieldValueProps) => {
const [fieldOpen, setFieldOpen] = useState(false);

const value = String(formattedValue);
Expand All @@ -30,11 +40,18 @@ export const TableFieldValue = ({ formattedValue, field }: TableFieldValueProps)

const onToggleCollapse = () => setFieldOpen((fieldOpenPrev) => !fieldOpenPrev);

const multiValue = Array.isArray(rawValue) && rawValue.length > 1;

return (
<Fragment>
{isCollapsible && (
<DocViewTableRowBtnCollapse onClick={onToggleCollapse} isCollapsed={isCollapsed} />
)}
{ignoreReason && (
<em>
{ignoreReason.toString()} (multiValue: {String(multiValue)})
</em>
)}
<div
className={valueClassName}
data-test-subj={`tableDocViewRow-${field}-value`}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,18 @@ export const MAIN_COLUMNS: Array<EuiBasicTableColumn<FieldRecord>> = [
</strong>
</EuiText>
),
render: ({ formattedValue }: FieldRecord['value'], { field: { field } }: FieldRecord) => {
return <TableFieldValue field={field} formattedValue={formattedValue} />;
render: (
{ formattedValue, ignored }: FieldRecord['value'],
{ field: { field }, action: { flattenedField } }: FieldRecord
) => {
return (
<TableFieldValue
field={field}
formattedValue={formattedValue}
rawValue={flattenedField}
ignoreReason={ignored}
/>
);
},
},
];
42 changes: 42 additions & 0 deletions src/plugins/discover/public/application/helpers/format_value.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* 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 { estypes } from '@elastic/elasticsearch';
import { DataView, DataViewField, KBN_FIELD_TYPES } from '../../../../data/common';
import { getServices } from '../../kibana_services';

// TODO: need more test coverage

/**
* Formats the value of a specific field using the appropriate field formatter if available
* or the default string field formatter otherwise.
*
* @param value The value to format
* @param hit The actual search hit (required to get highlight information from)
* @param dataView The data view if available
* @param field The field that value was from if available
*/
export function formatFieldValue(
value: unknown,
hit: estypes.SearchHit,
dataView?: DataView,
field?: DataViewField
): string {
if (!dataView || !field) {
// If either no field is available or no data view, we'll use the default
// string formatter to format that field.
return getServices()
.fieldFormats.getDefaultInstance(KBN_FIELD_TYPES.STRING)
.convert(value, 'html', { hit, field, indexPattern: dataView });
}

// If we have a data view and field we use that fields field formatter
return dataView
.getFormatterForField(field)
.convert(value, 'html', { hit, field, indexPattern: dataView });
}
Loading