Use locators service for Discover shared URLs #154947

lukasolson · 2023-04-13T18:55:31Z

Summary

Resolves #148886.
Resolves #142525.
Resolves #156275.
Resolves #142522.

Uses the URL locators service for generating the URL when clicking "Share" in Discover. This enables ad-hoc data views in shared URLs. As a result, we no longer show a prompt to save the ad-hoc data view before sharing, which means we can get rid of some no-longer used hooks.

Checklist

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

Release notes

Updates Discover sharing capabilities to enable sharing a link to Discover when using ad-hoc (temporary) data views.
Enables export to CSV when using ad-hoc data views.

elasticmachine · 2023-04-13T18:55:34Z

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

src/plugins/discover/public/application/main/components/top_nav/get_top_nav_links.tsx

src/plugins/discover/public/application/main/hooks/use_adhoc_data_views.ts

…fix'

lukasolson · 2023-04-20T21:21:49Z

I still need to update functional tests for this. I'm also running into a bug where shorten URLs aren't working initially... But this may be a bug with the short URL service directly. I'm still investigating.

lukasolson · 2023-04-21T23:08:42Z

@kertal Could you see if this is working now as expected? As far as I can tell it's working properly now.

kertal · 2023-04-24T07:32:06Z

@kertal Could you see if this is working now as expected? As far as I can tell it's working properly now.

Generally looking good 👍
One thing I noticed it seems before saved object sharing added the global filters :
Error: expected http://localhost:5620/app/discover#/view/ab12e3c0-f231-11e6-9486-733b1ac9221a?_g=() to equal http://localhost:5620/app/discover#/view/ab12e3c0-f231-11e6-9486-733b1ac9221a?_g=(filters%3A!()%2CrefreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3A\'2015-09-19T06%3A31%3A44.000Z\'%2Cto%3A\'2015-09-23T18%3A31%3A44.000Z\'))

The question is here, if the old behavior was right, since we communicate to the user we just share the URL to the saved version of the saved object?

src/plugins/discover/public/application/main/components/top_nav/get_top_nav_links.tsx

kertal · 2023-04-24T07:41:11Z

Hurray for removing code in this PR 🥳
One thing I wonder, does relativeToAbsolute add so much to our package?

vadimkibana

SharedUX code changes LGTM.

…fix'

lukasolson · 2023-05-01T16:25:01Z

I created the bug issue for pinned filters here: #156275

kertal · 2023-05-03T11:04:10Z

@elasticmachine merge upstream

kertal

This is great work 👍 ! Sometimes easy looking things turn into a 🐰🕳️ ! So it needs a gentle 🦊 like you to dig a bit deeper. One hopefully last thing I encountered while testing ... it seems it's not possible to create a CSV link with temporary data views, because the link gets to long? seems it contains lots of mappings?

If there's no workaround for this we might consider to not provide this button when using temporary data views

kertal · 2023-05-03T15:14:12Z

src/plugins/discover/public/application/main/components/top_nav/get_top_nav_links.tsx

+      // This logic is duplicated from `relativeToAbsolute` (for bundle size reasons). Ultimately, this should be
+      // replaced when https://github.com/elastic/kibana/issues/153323 is implemented.
+      const link = document.createElement('a');
+      link.setAttribute('href', relativeUrl);
+      const shareableUrl = link.href;


I think this needs to be updated, since this was merged, but doesn't seem to solve the issue

#153323 (comment)

I do agree with you, it should not be necessary to use this workaround. If getUrl works it should also be possible to use getRedirectUrlas an absolute URL .. FYI @vadimkibana

Anyway thx for taking care of the bundle size @lukasolson

lukasolson · 2023-05-16T20:01:36Z

One hopefully last thing I encountered while testing ... it seems it's not possible to create a CSV link with temporary data views, because the link gets to long? seems it contains lots of mappings?

This should be fixed now, do you mind giving it one more look? 🙏 🙇

tsullivan · 2023-05-16T21:38:07Z

src/plugins/discover/public/utils/get_sharing_data.ts

@@ -107,7 +107,7 @@ export async function getSharingData(

        searchSource.setField('fields', fields);
      }
-      return searchSource.getSerializedFields(true);
+      return searchSource.getSerializedFields(true, false);


@lukasolson should this be false only for ad-hoc data views?

I'm not sure why you'd want to send the field list/mappings in the request since they don't seem to be necessary to the POST URL. Is there something that is being accessed from this list in the CSV generation?

Reporting doesn't care about the parameters, as they just get forwarded to other dependencies on the server side when the report is generating. These are all called in the kbn-generate-csv package:

searchSource.getSearchRequestBody()

data.search()

field formatters

tabifyDocs()

If those dependencies do not need the field list or mappings, then I agree with you they don't seem to be necessary. It may have just been a carryover from an older implementation.

On a side note, I find the CSV export implementation that passes a searchsource object in the parameters to be overly complex, and since it doesn't reference a saved search it isn't user-friendly for advanced users that want to automate reports of any saved search. That's why I consider the csv_searchsource export type to be tech debt: #151190

kibana-ci · 2023-05-16T23:31:52Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	462	460	-2

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
`share`	59	60	+1

Async chunks

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

id	before	after	diff
`discover`	424.1KB	421.9KB	-2.2KB

Page load bundle

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

id	before	after	diff
`share`	51.5KB	51.8KB	+386.0B

Unknown metric groups

API count

id	before	after	diff
`share`	118	119	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	400	404	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	480	484	+4
total			+6

History

💔 Build #128388 failed 10eb19d
💚 Build #128368 succeeded 61f74ec
💛 Build #125104 was flaky a4bcb9a
💛 Build #124646 was flaky 1f757d1
💔 Build #124576 failed f1032ac

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

cc @lukasolson

kertal

LGTM, 🕺 Thx! Tested locally and also the CSV POST thingy now works. Once @tsullivan's question is clarified, pls ship it. Great work! Really excited we now fully support temporary data views and can remove those workarounds!

Use locators service for Discover shared URLs

0fbe836

lukasolson added release_note:enhancement Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.8.0 labels Apr 13, 2023

lukasolson requested a review from a team as a code owner April 13, 2023 18:55

lukasolson self-assigned this Apr 13, 2023

lukasolson added the ci:cloud-deploy Create or update a Cloud deployment label Apr 13, 2023

kertal reviewed Apr 17, 2023

View reviewed changes

src/plugins/discover/public/application/main/components/top_nav/get_top_nav_links.tsx Outdated Show resolved Hide resolved

kertal reviewed Apr 17, 2023

View reviewed changes

src/plugins/discover/public/application/main/hooks/use_adhoc_data_views.ts Outdated Show resolved Hide resolved

lukasolson and others added 6 commits April 17, 2023 14:13

Merge branch 'main' into fix/153323

3cab51a

Remove unused hooks

da20f92

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

521cabf

…fix'

Merge branch 'main' into fix/153323

d2af76e

Merge remote-tracking branch 'origin/fix/153323' into fix/153323

3b19321

Add support for saved searches

3599355

Fix support for short URLs

b47d38f

lukasolson requested a review from a team as a code owner April 21, 2023 23:07

kertal reviewed Apr 24, 2023

View reviewed changes

src/plugins/discover/public/application/main/components/top_nav/get_top_nav_links.tsx Show resolved Hide resolved

vadimkibana approved these changes Apr 25, 2023

View reviewed changes

lukasolson added 2 commits April 26, 2023 10:22

Merge branch 'main' into fix/153323

7b9dfd9

Move url generation to share service

985e522

lukasolson force-pushed the fix/153323 branch from 41a264f to 985e522 Compare April 26, 2023 19:13

kibanamachine and others added 2 commits April 26, 2023 19:18

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

7fff334

…fix'

Fix global filters in snapshot sharing

e02adbc

lukasolson and others added 5 commits April 28, 2023 16:11

Fix test

4bf2707

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

bf765ec

…fix'

Merge branch 'main' into fix/153323

de8cda8

Merge remote-tracking branch 'origin/fix/153323' into fix/153323

86befb0

Remove unused translations

d79e3dd

Merge branch 'main' into fix/153323

f1032ac

lukasolson added v8.9.0 and removed v8.8.0 labels May 1, 2023

Merge branch 'main' into fix/153323

1f757d1

lukasolson marked this pull request as ready for review May 1, 2023 23:00

Merge branch 'main' into fix/153323

a4bcb9a

kertal self-requested a review May 3, 2023 15:05

kertal reviewed May 3, 2023

View reviewed changes

lukasolson added 2 commits May 16, 2023 11:30

Merge branch 'main' into fix/153323

61f74ec

Don't include field mappings in URL job params

10eb19d

tsullivan reviewed May 16, 2023

View reviewed changes

Add back filter validation

89276ba

kertal self-requested a review May 17, 2023 06:47

kertal approved these changes May 17, 2023

View reviewed changes

lukasolson merged commit 9359c63 into elastic:main May 17, 2023

kibanamachine added the backport:skip This commit does not require backporting label May 17, 2023

lukasolson added the release_note:fix label May 17, 2023

lukasolson mentioned this pull request May 17, 2023

[Discover] enable reporting for ad-hoc dataviews #142522

Closed

This was referenced May 22, 2023

[Discover] Rename internalState to sharedState #157830

Closed

[Discover] Remove state persistAdHocDataView #158227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use locators service for Discover shared URLs #154947

Use locators service for Discover shared URLs #154947

lukasolson commented Apr 13, 2023 •

edited

Loading

elasticmachine commented Apr 13, 2023

lukasolson commented Apr 20, 2023

lukasolson commented Apr 21, 2023

kertal commented Apr 24, 2023

kertal commented Apr 24, 2023

vadimkibana left a comment

lukasolson commented May 1, 2023

kertal commented May 3, 2023

kertal left a comment

kertal May 3, 2023

kertal May 3, 2023

lukasolson commented May 16, 2023

tsullivan May 16, 2023

lukasolson May 16, 2023

tsullivan May 17, 2023 •

edited

Loading

kibana-ci commented May 16, 2023 •

edited

Loading

API count

ESLint disabled line counts

Total ESLint disabled count

kertal left a comment

Use locators service for Discover shared URLs #154947

Use locators service for Discover shared URLs #154947

Conversation

lukasolson commented Apr 13, 2023 • edited Loading

Summary

Checklist

For maintainers

Release notes

elasticmachine commented Apr 13, 2023

lukasolson commented Apr 20, 2023

lukasolson commented Apr 21, 2023

kertal commented Apr 24, 2023

kertal commented Apr 24, 2023

vadimkibana left a comment

Choose a reason for hiding this comment

lukasolson commented May 1, 2023

kertal commented May 3, 2023

kertal left a comment

Choose a reason for hiding this comment

kertal May 3, 2023

Choose a reason for hiding this comment

kertal May 3, 2023

Choose a reason for hiding this comment

lukasolson commented May 16, 2023

tsullivan May 16, 2023

Choose a reason for hiding this comment

lukasolson May 16, 2023

Choose a reason for hiding this comment

tsullivan May 17, 2023 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented May 16, 2023 • edited Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

kertal left a comment

Choose a reason for hiding this comment

lukasolson commented Apr 13, 2023 •

edited

Loading

tsullivan May 17, 2023 •

edited

Loading

kibana-ci commented May 16, 2023 •

edited

Loading