Deprecate DataView.flattenHit in favor of data plugin flattenHit #114517

timroes · 2021-10-11T15:38:07Z

Summary

We currently have too many flattenHit implementations in Kibana. After talking with @ppisljar we decided to deprecate the one in ~~index patterns~~ data views, and instead get people to use the one from the data plugin (from tabifyDocs).

This PR solves solves those things:

Deprecate the flattenHit implementation in data view in favor of the one from the data plugin.
Make Discover use the flattenHit from the data plugin.
Remove the flattenHitWrapper API that was only used in Discover from the data plugin.

There are a couple of changes I made, please have a look at my inline comments in this PR, but the actual functionality should not be affected by this PR and the expectations would be, that everything works the same as beforehand.

⚠️ There is one main difference that I did not moved over to the new implementation. The old implementation converted arrays of single elements into flat values (relevant code). I could not find any difference in behavior without this while testing different field formatters, filtering, etc. Thus I decided to leave it out, unless someone still knows why this would be needed.

For reviewers: Ideally hide whitespace changes when reviewing code, there are a lot of indentation changes.
For testers: To test this, you should make sure documents in Discover are still shown correctly under all circumstances. Also please make sure CSV export is still working as expected.

Checklist

Delete any items that are not applicable to this PR.

~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~
~~[ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~[ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list~~
~~[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
~~[ ] This was checked for cross-browser compatibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~

timroes · 2021-10-12T15:29:41Z

@elasticmachine merge upstream

…/kibana into dataview/flatten-hit-deprecation

timroes · 2021-10-13T07:34:26Z

@elasticmachine merge upstream

timroes · 2021-10-13T09:19:20Z

src/plugins/data/common/search/tabify/tabify_docs.test.ts

-    hits: {
-      hits: [
+describe('tabify_docs', () => {
+  describe('flattenHit', () => {


ℹ️ I've moved the tests from the flattenHit implementation in data view over here, so they will also be applied to this implementation.

timroes · 2021-10-13T09:19:59Z

src/plugins/data/common/search/tabify/tabify_docs.test.ts

+          },
+        },
+      },
+      metaFields: ['_id', '_index', '_score', '_type'],


ℹ️ Since the implementation now uses the metaFields from the index pattern (which are set from the metaFields advanced setting), tests must correctly set those.

timroes · 2021-10-13T09:21:21Z

src/plugins/data/common/search/tabify/tabify_docs.ts

-  return flat;
+  // 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) => {


ℹ️ So far the implementation in the data plugin just hard-coded loaded a couple of meta fields. We have an advanced setting for this, that should be used, so changing this implementation to read the metaFields from the index pattern (which is fed by that advanced setting - and also will leave us the possibility to later make that an index pattern level setting).

timroes · 2021-10-13T09:22:06Z

src/plugins/data/common/search/tabify/tabify_docs.ts

+  // 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) => {
+    if (!isValidMetaFieldName(metaFieldName) || metaFieldName === '_source') {


ℹ️ I am filtering out for valid metaField names now. The data view implementation did not do that. This will prevent TypeScript from complaining here, and also potentially add a bit safetyness to not mess up objects completely.

timroes · 2021-10-13T09:23:01Z

src/plugins/data/common/search/tabify/tabify_docs.ts

+
+  // Use a proxy to make sure that keys are always returned in a specific order,
+  // so we have a guarantee on the flattened order of keys.
+  return new Proxy(flat, {


ℹ️ This has been added via #93344 and so I also added it to this implementation now.

timroes · 2021-10-13T09:24:18Z

...scover/public/application/components/discover_grid/discover_grid_document_selection.test.tsx

@@ -17,6 +17,17 @@ import { esHits } from '../../../__mocks__/es_hits';
 import { indexPatternMock } from '../../../__mocks__/index_pattern';
 import { DiscoverGridContext } from './discover_grid_context';

+const baseContextMock = {


ℹ️ This is a left over of an slightly different implementation I went with originally, but since it's deduplicating some code, I decided to leave it in this PR.

timroes · 2021-10-13T09:25:34Z

src/plugins/data/common/search/tabify/tabify_docs.ts

  source?: boolean;
-  meta?: boolean;


ℹ️ I've removed this parameter. All places that are using this are requiring meta to be loaded. Also I couldn't find any place we flatten documents in Kibana, where we don't want this. So removing this for now, until we really have a use-case for this.

timroes · 2021-10-13T09:33:28Z

src/plugins/discover/public/application/components/discover_grid/get_render_cell_value.test.tsx

+            "__html": Array [
+              100,
+            ],


ℹ️ This change in snapshot is caused by the change described in the PR, of single element arrays no longer being unwrapped.

this is a more accurate behavior 👍

elasticmachine · 2021-10-13T09:33:59Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

ppisljar

code LGTM

timroes · 2021-10-13T12:51:31Z

Buildkite, test this

Anyway waiting for review, so I try to turn this 💛 into a 💚

majagrubic · 2021-10-13T14:21:55Z

I've tested this quite extensively and all seems to be working fine. Working through the code still.

kibanamachine · 2021-10-13T14:40:06Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`data`	2807	2803	-4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	327.7KB	327.8KB	+76.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	465.1KB	465.5KB	+444.0B
`dataViews`	38.6KB	38.5KB	-86.0B
total			+358.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`dataViews`	228	236	+8
`discover`	1821	1801	-20
`maps`	1689	1697	+8
total			-4

History

💛 Build #266 was flaky f890df2
💔 Build #241 failed 921bd6c
💔 Build #234 failed 5c47379
💔 Build #231 failed 07c2231
💔 Build #228 failed 76e44ae
💔 Build #175 failed 9765492

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

jloleysens

I read the code for reporting and changes LGTM. It's a little tricky to evaluate whether there is a regression since I'm not very familiar with flattenHit but I did a local test of CSV generation it worked as expected 👍🏻 .

jloleysens · 2021-10-14T12:41:31Z

x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.test.ts

@@ -75,6 +75,7 @@ const mockSearchSourceGetFieldDefault = jest.fn().mockImplementation((key: strin
          getByName: jest.fn().mockImplementation(() => []),
          getByType: jest.fn().mockImplementation(() => []),
        },
+        metaFields: ['_id', '_index', '_type', '_score'],


I did a grep on the code, looks like these are still present on the deprecated csv export type that is using another implementation of flattenHits that I am guessing will be removed at a future date when that export type is removed.

Yeah, the legacy csv implementation actually is something we need/started discussing parallel to this, if this still needs to be adjusted to changes we will make from now on to this, or if we're fine with them diverging from each other.

kibanamachine · 2021-10-14T12:46:14Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 114517

…4517) (#114991) * WIP replacing indexPattern.flattenHit by tabify * Fix jest tests * Read metaFields from index pattern * Remove old test code * remove unnecessary changes * Remove flattenHitWrapper APIs * Fix imports * Fix missing metaFields * Add all meta fields to allowlist * Improve inline comments * Move flattenHit test to new implementation * Add deprecation comment to implementation Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # src/plugins/data_views/public/index.ts

WIP replacing indexPattern.flattenHit by tabify

9765492

timroes added Feature:Discover Discover Application buildkite-ci Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Oct 11, 2021

Tim Roes added 7 commits October 12, 2021 12:46

Fix jest tests

76e44ae

Read metaFields from index pattern

4304f55

Remove old test code

13bb111

remove unnecessary changes

07c2231

Remove flattenHitWrapper APIs

983f830

Fix imports

8be2525

Fix missing metaFields

5c47379

kibanamachine and others added 3 commits October 12, 2021 11:29

Merge branch 'master' into dataview/flatten-hit-deprecation

8292e0c

Add all meta fields to allowlist

6ebbf26

Merge branch 'dataview/flatten-hit-deprecation' of github.com:timroes…

921bd6c

…/kibana into dataview/flatten-hit-deprecation

kibanamachine and others added 4 commits October 13, 2021 03:34

Merge branch 'master' into dataview/flatten-hit-deprecation

066c7aa

Improve inline comments

5349298

Move flattenHit test to new implementation

911cf9e

Add deprecation comment to implementation

f890df2

timroes commented Oct 13, 2021

View reviewed changes

timroes added v7.16.0 v8.0.0 labels Oct 13, 2021

timroes added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes labels Oct 13, 2021

timroes marked this pull request as ready for review October 13, 2021 09:33

timroes requested review from a team as code owners October 13, 2021 09:33

timroes changed the title ~~[Discover] Deprecate indexPattern.flattenHit [skip ci]~~ [Discover] Deprecate indexPattern.flattenHit Oct 13, 2021

timroes changed the title ~~[Discover] Deprecate indexPattern.flattenHit~~ Deprecate DataView.flattenHit in favor of data plugin flattenHit Oct 13, 2021

ppisljar approved these changes Oct 13, 2021

View reviewed changes

majagrubic approved these changes Oct 14, 2021

View reviewed changes

jloleysens approved these changes Oct 14, 2021

View reviewed changes

timroes merged commit b21e1eb into elastic:master Oct 14, 2021

timroes deleted the dataview/flatten-hit-deprecation branch October 14, 2021 12:43

timroes mentioned this pull request Oct 14, 2021

[7.x] Deprecate DataView.flattenHit in favor of data plugin flattenHit (#114517) #114991

Merged

kertal mentioned this pull request Dec 16, 2021

[Discover] Using the mapper_size (ES plugin) _size is missing in the document table #121394

Closed

kertal mentioned this pull request Jan 17, 2022

[Discover] Using _source fields of objects are no longer displayed separately #123169

Closed

kertal mentioned this pull request Jul 27, 2022

[Discover] Fix filtering out custom meta fields of Elasticsearch plugins enhanced documents #137147

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate DataView.flattenHit in favor of data plugin flattenHit #114517

Deprecate DataView.flattenHit in favor of data plugin flattenHit #114517

timroes commented Oct 11, 2021 •

edited

Loading

timroes commented Oct 12, 2021

timroes commented Oct 13, 2021

timroes Oct 13, 2021

timroes Oct 13, 2021

timroes Oct 13, 2021

timroes Oct 13, 2021

timroes Oct 13, 2021

timroes Oct 13, 2021

timroes Oct 13, 2021

timroes Oct 13, 2021

kertal Oct 13, 2021

elasticmachine commented Oct 13, 2021

ppisljar left a comment

timroes commented Oct 13, 2021 •

edited

Loading

majagrubic commented Oct 13, 2021

kibanamachine commented Oct 13, 2021

References to deprecated APIs

jloleysens left a comment

jloleysens Oct 14, 2021

timroes Oct 14, 2021

kibanamachine commented Oct 14, 2021

Deprecate DataView.flattenHit in favor of data plugin flattenHit #114517

Deprecate DataView.flattenHit in favor of data plugin flattenHit #114517

Conversation

timroes commented Oct 11, 2021 • edited Loading

Summary

Checklist

For maintainers

timroes commented Oct 12, 2021

timroes commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 13, 2021

ppisljar left a comment

Choose a reason for hiding this comment

timroes commented Oct 13, 2021 • edited Loading

majagrubic commented Oct 13, 2021

kibanamachine commented Oct 13, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

References to deprecated APIs

History

jloleysens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Oct 14, 2021

💔 Backport failed

timroes commented Oct 11, 2021 •

edited

Loading

timroes commented Oct 13, 2021 •

edited

Loading