[Lens] Show underlying data editor navigation #125983

dej611 · 2022-02-17T18:01:47Z

Summary

Fixes part of #122631

The main part of this PR is the availability of 2 main functions here that can be composed:

getLayerMetaInfo to extract all relevant information to navigate to Discover from a Lens state
combineQueryAndFilters to combine all meta information into the query and filters states to pass to the navigation.
The following is a diagram of the flow enabled by this PR (dotted lines means that they haven't been implemented yet):

flowchart LR
  A[Dashboard] -.-> |timeRange| E["navigate( ... )"]
  A[Dashboard] -.->|attributes| F[Embeddable]
  A[Dashboard] -.->|query, filters| B["combineQueryAndFilters( ... )"]
  F[Embeddable] -.->|state| D["getLayerMetaInfo( ... )"]
  C[Lens Editor] -->|state| D["getLayerMetaInfo( ... )"]
  D[getLayerMetaInfo] -->|"{ meta.filters }"| B["combineQueryAndFilters( ... )"]
  C[Lens Editor] -->|query, filters| B["combineQueryAndFilters( ... )"]
  D[getLayerMetaInfo] --> |"{ meta.id, meta.columns }"| E["navigate/getRedirectUrl( ... )"]
  C[Lens Editor] --> |timeRange| E["navigate/getRedirectUrl( ... )"]
  B["combineQueryAndFilters( ... )"] -->|query, filters| E["navigate/getRedirectUrl( ... )"]

In the editor the button is shown as first on the left:

When the configuration is not supported the button is disabled:

When Discover is disabled (either navLinks or discover.show capabilities ) then the button is not displayed at all:

Notes
This PR does not include the embeddable action yet, providing only the foundation for the main task and an initial testing playground with the editor button.

Known bugs

Create a XY visualization, add a reference layer and configure it, remove the reference layer. Button is still disabled.
- Apparently the activeData still contains the reference layer data in it, making the check fail at the multiple layers step

Some questions

What label should have the button in the editor? ✅
- Show underlying data is pretty long. At the moment Open in Discover has been used, but open to suggestions here.
What should happen when no fields are present in the configuration? (I.e. Count of records?) ✅
- I think it still makes sense to show the data underneath with the default columns (Document). It is currently enabled as described.
What should happen for configuration with (at least one) time shift? ✅
- It might make sense to show the configuration for all the data without the time shift? At the moment the link is disabled in at least one time shift is detected.
What should happen when there is a reference layer with only a static value?
- Maybe it's worth considering to enable this specific case? Or perhaps consider to complete ignore reference layers from the computation check? At the moment there's no layer distinction and as soon as a reference layer is created the button is disabled.
Makes sense IMHO to check for fully static layers and exclude them from consideration
- Mark it as a followup ➡️
How to get back to the visualization configuration once navigated to Discover? ✅
- Right now there's only the browser history to support that, which relies on the persisted state (i.e. last saved state or empty). I think it makes sense to have a Go back to Lens button in Discover which helps the user to get back to complete/edit its configuration.
- Decided to make the link open a new tab and resolve this problem from the source.
What name should have the custom filter built with Lens aggregated filters? ✅
- I think it makes sense to have a specific name to make understand the user that the filter comes from the Lens context. At the moment a Lens context ($language) custom label is used.
What should happen for transposed datatables? ✅
- While I think it makes sense to make it as much transparent as possible to the user the transposition process, there's an implementation limit to make this work. At the moment transposed dimensions are falling back to an existing filter check rather than specific field: term values.

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

…ying-data

elasticmachine · 2022-02-22T15:30:36Z

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

dej611 · 2022-02-23T10:22:02Z

@elasticmachine merge upstream

dej611 · 2022-02-23T11:02:01Z

@elasticmachine merge upstream

…na into feature/show-underlying-data

dej611 · 2022-03-02T17:11:01Z

Found a case that doesn't work right: If there are two metrics in a layer, one of them filtered and the other one not filtered, it should discard the filters from the other metric - right now it's behaving as if both metrics have the same filter. E.g. a formula count(kql="some filter") / count() should not add "some filter" to the query because the second count() takes all data into account.

Managed to properly fix this issue and also had some time to think about the user flow in this case.
While having a non-filtered metric on a visualization configuration amongst filtered metrics indicates that the user is interested in the wider set of documents (all of them), this does not mean that the smaller sets are an information that we should ignore.

For instance let's assume this (bizarre) formula: average(bytes, kql='bytes > 6000') / average(bytes):

Redirecting the user to discover now will show up an empty search box with the bytes field selected:

While this is technically right, I think that we missed some informations along the way, in particular the interest of the user for smaller sets of documents. Ideally, I think we should preserve this information providing "disabled" distinct filters (other than the Lens context one), that the user could toggle on and off to perform some contextual investigations:

Of course this is not something we need to have since day one, but I see it as an interesting enhancement of this feature as follow up. What do you think @flash1293 @ghudgins ?

drewdaemon · 2022-03-03T15:48:20Z

x-pack/plugins/lens/public/app_plugin/lens_top_nav.tsx

@@ -290,6 +313,26 @@ export const LensTopNavMenu = ({
    filters,
    initialContext,
  ]);
+
+  const canShowUnderlyingData = useMemo(() => {


This name implies a boolean value. Maybe layerMetaInfo?

dej611 · 2022-03-04T09:59:21Z

@elasticmachine merge upstream

drewdaemon · 2022-03-04T22:46:44Z

Just noticed that the "Open in Discover" button remains grayed-out when I remove the second layer:

Screen.Recording.2022-03-04.at.4.45.01.PM.mov

dej611 · 2022-03-07T08:14:11Z

Just noticed that the "Open in Discover" button remains grayed-out when I remove the second layer:

Yes, that was reported in the known issues section.
There's this bug #126886 which affects not only this feature, but also the Inspector table and the CSV download.

flash1293 · 2022-03-07T08:38:04Z

@elasticmachine merge upstream

flash1293

This looks almost good to me, just one small thing I noticed: Single quotes are not encoded correctly, leading to "no results found":

Approving to not holding you back (I think the encoding thing can also be done as a follow-up)

flash1293 · 2022-03-07T09:50:53Z

x-pack/plugins/lens/public/app_plugin/show_underlying_data.ts

+  }
+
+  const uniqueFields = [...new Set(columnsWithNoTimeShifts.map(({ fields }) => fields).flat())];
+  // If no field, return? Or rather carry on and show the default columns?


Is this comment still neccessary?

flash1293 · 2022-03-07T09:52:06Z

x-pack/plugins/lens/public/app_plugin/show_underlying_data.ts

+    layerId: firstLayerId,
+    state: datasourceState,
+  });
+  // maybe add also datasourceId validation here?


Let's put it in for now, I think long term we can make it work for other datasources as well.

dej611 · 2022-03-07T11:46:23Z

This looks almost good to me, just one small thing I noticed: Single quotes are not encoded correctly, leading to "no results found":

Had to reduce the escaping logic to make this work. Now only empty filter is properly escaped.
I've tried to break it with some values containing brackets, without results, but I think there's already some logic for it by the query bar.

dej611 · 2022-03-08T16:43:21Z

@elasticmachine merge upstream

kibana-ci · 2022-03-08T18:19:16Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	748	749	+1

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
`navigation`	32	34	+2

Async chunks

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

id	before	after	diff
`lens`	1.1MB	1.1MB	+6.2KB

Page load bundle

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

id	before	after	diff
`navigation`	9.3KB	9.4KB	+69.0B

Unknown metric groups

API count

id	before	after	diff
`navigation`	32	34	+2

History

💚 Build #27990 succeeded b464915
💔 Build #27961 failed bda6a9d
💚 Build #27912 succeeded 51da4b9
💚 Build #27618 succeeded 0ad4bc3
💔 Build #27593 failed 5f727c7
💔 Build #27446 failed 7be3ec7

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

…re-browser-errors * 'main' of github.com:elastic/kibana: (46 commits) [Reporting] Capture Kibana stopped error (elastic#127017) add updatedAt to SimpleSavedObject (elastic#126359) Remove deprecated & unused `ElasticsearchServiceStart.legacy` (elastic#127050) remove opacity for fitting line series (elastic#127176) Remove deprecated & unused `HttpServiceSetup.auth` (elastic#127056) [Lens] Show underlying data editor navigation (elastic#125983) Bump dependencies (elastic#127238) Remove deprecated & unused `public-AsyncPlugin` (elastic#127048) Remove deprecated & unused `SavedObjectsImportFailure.title` (elastic#127043) skip flaky suite (elastic#123372) [kbn/generate] add basic package generator (elastic#127095) [build] Up compression quality (elastic#127064) Made fix to broken test. Deleted all existing pipelines before test starts. FLAKY: elastic#118593 (elastic#127102) Increase timeout for Jest integration tests (elastic#127220) skip failing test suite (elastic#126949) [DOCS] Adds note for data source performance impact (elastic#127184) [Security Solution] Adds CCS privileges warning enable switch in advanced settings (elastic#124459) [App Search] Move to tabbed single tabbed JSON flyout with upload and paste options and refactor cards (elastic#127162) Update dependency chromedriver to v99 (elastic#127079) [kbn/pm] add timings for more parts of bootstrap (elastic#127157) ... # Conflicts: # x-pack/plugins/reporting/common/errors/index.ts # x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

dej611 added 5 commits February 15, 2022 09:54

⚗️ First steps

8805136

Merge remote-tracking branch 'upstream/main' into feature/show-underl…

873ff6d

…ying-data

🎉 Initial implementation for button insider editor

fa64071

✅ fix types and tests

32baaa0

✅ Add some tests and some test placeholders

f659b6b

dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Feb 17, 2022

dej611 added 6 commits February 21, 2022 09:38

Merge remote-tracking branch 'upstream/main' into feature/show-underl…

7495712

…ying-data

🐛 Fix issues on mount

54bdbd2

🔥 Remove unused attr

5b7dbe4

✅ Add more tests for edge cases

c938b01

♻️ First code refactor

05163c8

🐛 Fix discover capabilities check

6206d27

dej611 added ci:deploy-cloud v8.2.0 labels Feb 21, 2022

dej611 changed the title ~~[Lens][WIP] Show underlying data editor navigation~~ [Lens] Show underlying data editor navigation Feb 21, 2022

dej611 removed the ci:deploy-cloud label Feb 22, 2022

dej611 added 3 commits February 22, 2022 13:03

🐛 Fix various issues

675b4db

✅ Add functional tests

9dfc9d2

Merge remote-tracking branch 'upstream/main' into feature/show-underl…

6ddbee4

…ying-data

dej611 added the ci:deploy-cloud label Feb 22, 2022

✅ Add more tests

aec1234

dej611 added backport:skip This commit does not require backporting release_note:enhancement labels Feb 22, 2022

dej611 marked this pull request as ready for review February 22, 2022 15:30

dej611 requested a review from a team as a code owner February 22, 2022 15:30

Merge branch 'main' into feature/show-underlying-data

b214e75

Merge branch 'feature/show-underlying-data' of github.com:dej611/kiba…

f0f7df5

…na into feature/show-underlying-data

dej611 added 3 commits March 3, 2022 09:27

✅ Fix functional tests for formula case

1f6682a

🔧 Make close editor panel more robust

972b432

🔧 Try focus approach

6eec900

drewdaemon reviewed Mar 3, 2022

View reviewed changes

dej611 added 3 commits March 3, 2022 18:21

👌 Rename variable

7be3ec7

✅ Try to click outside

dbf8de9

✅ Use the keyboard arrow approach

5f727c7

Merge branch 'main' into feature/show-underlying-data

0ad4bc3

Merge branch 'main' into feature/show-underlying-data

51da4b9

flash1293 approved these changes Mar 7, 2022

View reviewed changes

👌 Address some issues raised by review

bda6a9d

✅ Fix tests and add one more for unsupported datasource

b464915

Merge branch 'main' into feature/show-underlying-data

6f36675

dej611 merged commit 133e57f into elastic:main Mar 9, 2022

This was referenced Mar 9, 2022

[Lens] Show underlying data embeddable dej611/kibana#15

Closed

[Lens] Add disabled filters when show underlying data #127253

Merged

dej611 mentioned this pull request Mar 16, 2022

Add ability to go from a selected item in a visualization to Discover #127780

Closed

KOTungseth mentioned this pull request Mar 30, 2022

Adds the Lens 8.2.0 docs #128986

Merged

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Show underlying data editor navigation #125983

[Lens] Show underlying data editor navigation #125983

dej611 commented Feb 17, 2022 •

edited

elasticmachine commented Feb 22, 2022

dej611 commented Feb 23, 2022

dej611 commented Feb 23, 2022

dej611 commented Mar 2, 2022

drewdaemon Mar 3, 2022

dej611 commented Mar 4, 2022

drewdaemon commented Mar 4, 2022 •

edited

dej611 commented Mar 7, 2022

flash1293 commented Mar 7, 2022

flash1293 left a comment •

edited

flash1293 Mar 7, 2022

flash1293 Mar 7, 2022

dej611 commented Mar 7, 2022

dej611 commented Mar 8, 2022

kibana-ci commented Mar 8, 2022

API count

[Lens] Show underlying data editor navigation #125983

[Lens] Show underlying data editor navigation #125983

Conversation

dej611 commented Feb 17, 2022 • edited

Summary

Checklist

elasticmachine commented Feb 22, 2022

dej611 commented Feb 23, 2022

dej611 commented Feb 23, 2022

dej611 commented Mar 2, 2022

drewdaemon Mar 3, 2022

Choose a reason for hiding this comment

dej611 commented Mar 4, 2022

drewdaemon commented Mar 4, 2022 • edited

dej611 commented Mar 7, 2022

flash1293 commented Mar 7, 2022

flash1293 left a comment • edited

Choose a reason for hiding this comment

flash1293 Mar 7, 2022

Choose a reason for hiding this comment

flash1293 Mar 7, 2022

Choose a reason for hiding this comment

dej611 commented Mar 7, 2022

dej611 commented Mar 8, 2022

kibana-ci commented Mar 8, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

dej611 commented Feb 17, 2022 •

edited

drewdaemon commented Mar 4, 2022 •

edited

flash1293 left a comment •

edited