[App Search] Fixed Documents view for Editor and Analyst roles #103113

JasonStoltz · 2021-06-23T14:59:02Z

Summary

This change updates the Documents view search to rely on basic authentication rather than a search key.

As part of that, we needed to create a proxy endpoint for search-ui, rather than going directly to the ent-search API.

I updated to search-ui 1.6.0, which adds the ability to use basic auth, which we need for this fix.

For maintainers

This was checked for breaking API changes and was labeled appropriately

1.6.0 adds the ability to authenticate without a search key, which we need for this fix.

This change updates the page to rely on basic authentication rather than a search key. As part of that, we needed to create a proxy endpoint for search-ui to post to, rather than going directly to the ent-search API.

cee-chen

Pulling this down for a quick QA and then approving! Awesome work!

cee-chen · 2021-06-23T15:53:42Z

.../public/applications/app_search/components/documents/search_experience/search_experience.tsx

@@ -52,7 +52,8 @@ const DEFAULT_SORT_OPTIONS: SortOption[] = [

 export const SearchExperience: React.FC = () => {
  const { engine } = useValues(EngineLogic);
-  const endpointBase = externalUrl.enterpriseSearchUrl;
+  const { http } = useValues(HttpLogic);
+  const endpointBase = http.basePath.prepend('/api/app_search/search-ui');


nice find on this! 💯

cee-chen · 2021-06-23T15:53:58Z

.../public/applications/app_search/components/documents/search_experience/search_experience.tsx

+    additionalHeaders: {
+      'kbn-xsrf': true,
+    },


Similarly awesome find, hope this wasn't a huge headache to figure out 🤔

cee-chen · 2021-06-23T15:56:45Z

x-pack/plugins/enterprise_search/server/routes/app_search/search.ts

+  // Search UI *also* uses the click tracking and query suggestion endpoints, however, since the
+  // App Search plugin doesn't use that portion of Search UI, we only set up a proxy for the search endpoint below.


Sorry, I think my brain's a little fried this morning but just want to confirm I'm understanding this comment. By us not explicitly providing the click tracking and query suggestion routes in this PR, Search UI will no longer hit those endpoints. Is that correct?

From my own testing:

It looks like Analytics search/query events are still getting logged

However, Analytics click events are not getting logged, leading to inaccurate Analytics results when only using the dashboard

It also it looks like API events are no longer getting logged, although this could be my local behaving strangely

@JasonStoltz I think the click event thing is potentially a blocker unfortunately, it prevents us from QAing Analytics accurately using the Documents view (important for things like E2E testing also). Is there any way we can update Search UI to either track clicks again, or (another option if that's easier) to completely not affect Analytics whatsoever?

Just want to add also: I personally don't think this fix needs to get in by 7.14FF (or it's also OK if we take a little longer and get it in past FF, since it's a bugfix). It's been broken on ent-search for several minors now, and this is shipping as a beta UI, so comes with even less support - so I'm OK taking a little longer to figure this out for the long term and to help solidify our E2E tests.

This may even be a decent time also to discuss/address the tech debt item we've discussed in the past about whether dashboard API calls should count towards Analytics/API events or not.

Sorry, I think my brain's a little fried this morning but just want to confirm I'm understanding this comment. By us not explicitly providing the click tracking and query suggestion routes in this PR, Search UI will no longer hit those endpoints. Is that correct?

Documents view is not configured to use click tracking or query suggestions. Since it never hits those endpoints, we do not need to create proxy endpoints here.

It looks like Analytics search/query events are still getting logged

We should fix this, I don't think they should be logged

However, Analytics click events are not getting logged, leading to inaccurate Analytics results when only using the dashboard

I think the endgame is to not log any of these queries or clicks.

It also it looks like API events are no longer getting logged, although this could be my local behaving strangely

I don't know anything about that.

I think the click event thing is potentially a blocker unfortunately, it prevents us from QAing Analytics accurately using the Documents view (important for things like E2E testing also).

That behavior isn't introduced in this PR, so I don't think this is a blocker for this PR. It is a separate issue entirely.

Is there any way we can update Search UI to either track clicks again, or (another option if that's easier) to completely not affect Analytics whatsoever?

I think the answer is the latter. Any views in the Dashboard that hit the search endpoint Query Tester, Curations, Documents View ... none of them should be logging Analytics events, they don't provide any value to the user. This is something we discussed as part of the Search UI view transition.

I'd propose a simple header that indicates "this request should not be tracked".

Just want to add also: I personally don't think this fix needs to get in by 7.14FF (or it's also OK if we take a little longer and get it in past FF, since it's a bugfix). It's been broken on ent-search for several minors now, and this is shipping as a beta UI, so comes with even less support - so I'm OK taking a little longer to figure this out for the long term and to help solidify our E2E tests.

Just highlighting again that this PR is completely decoupled from that issue. If we want to enable click tracking we can, we just need to add a proxy endpoint for the click API route.

I think we should get this merged and fixed and handle analytics separately. I'm happy to create an issue for it in our backlog.

That behavior isn't introduced in this PR, so I don't think this is a blocker for this PR. It is a separate issue entirely.

To be clear, it is introduced by this PR (I'm not sure why though). It's not a separate issue unfortunately.

On Kibana master and on ent-search, you can do the following:

Go to the Documents view

Type in "park" (or any search that has results)

Click on any document

Go to Analytics

Confirm that your "park" query shows up in Analytics in "Top queries" and in "Top queries with clicks" indicating that the click event triggered

On this branch, if you repeat the above 4 steps the "Top queries with clicks" panel does not update, indicating that click events stopped being recorded.

Unfortunately, even if this is a side effect of this PR, it's not one I feel 100% comfortable creating so close to FF since it's a distinct deviation from how ent-search currently behaves. FWIW, I do agree the best long term solution is to not have any dashboard actions show up in either Analytics or API logs - I'd just like to take the extra effort to get that in if possible 🤔

I think we should get this merged and fixed and handle analytics separately. I'm happy to create an issue for it in our backlog.

Sorry, should have addressed this too. I'm OK with merging this in now only if we commit to fully handling (i.e., ignoring) dashboard analytics before 7.14 (so I guess after FF but before release). Otherwise, I think we should wait for 7.15. I really really worry about releasing this to 7.14 with an entire Analytics category (queries with clicks) incorrectly affected by our own dashboard in a way that it has not done so previously.

Would definitely appreciate it if you could try to repro my above steps and confirm behavior in master vs this branch! If I'm totally wrong about what's happening and click events are still being logged, then I'm GTG on this

I cannot reproduce.

Both on master kibana and this branch, clicking on a link will not send click data.

Inspect the network tab and you'll see for yourself, no API call is being made.

We did track these clicks in ent-search.

We do not, and never have tracked click analytics in the Documents view within Kibana. This PR does not change that. It doesn't prevent us from doing it in the future either. This PR is completely decoupled from your Analytics concern.

Whether we merge this PR or don't, it has no impact on Analytics.

Gah, yup, you're totally right. I just tried again on Kibana master with a fresh instance and clicks aren't showing up. Not sure if I was in a horrible fever dream, if my local elasticsearch instance got messed up, or if I got confused with ent-search 😰 (or all of the above?)

Thanks so much for taking the extra time to repro and confirm for me, I really appreciate that. Going to ahead and approve, and bring up filtering out dashboard analytics at next week's sync

After some thought I feel like the above comment does not convey my feeling of "Sorry for being totally wrong" enough so I'll fall back to my normal communication crutch of funny gifs

Haha, thanks for calling it out. Better to call it out and be wrong than say nothing.

We can definitely solve this Analytics thing. Maybe just enable click analytics for 7.14 and remove all analytics for 7.15.

cee-chen · 2021-06-23T16:07:50Z

x-pack/plugins/enterprise_search/server/routes/app_search/search.ts

+  // App Search plugin doesn't use that portion of Search UI, we only set up a proxy for the search endpoint below.
+  router.post(
+    skipBodyValidation({
+      path: '/api/app_search/search-ui/api/as/v1/engines/{engineName}/search.json',


Actually sorry, just wanted to leave a comment on this: do we really need to version this or make it look exactly like AS's endpoint? Any thoughts on

Suggested change

path: '/api/app_search/search-ui/api/as/v1/engines/{engineName}/search.json',

path: '/api/app_search/engines/{engineName}/search-ui/search.json',

Instead to better match our other Kibana API endpoints?

Sorry, I wondered if my comment above wasn't clear, I think this is evidence that it was not.

Search UI always posts its search query to:
{whatever base path you provide in configuration}/api/as/v1/engines/{engineName}/search.json.

EDIT: So yes, the last part of this path DOES need to match exactly, unfortunately.

I could update Search UI and the javascript client to accept a full path as configuration, and not just the base path, but that would be the long fix. I didn't anticipate this issue the last time I updated search-ui.

Ohhh gotcha gotcha. No worries in that case, my bad for failing to understand!

It's not your bad, it's mine for poor writing 😢

cee-chen · 2021-06-23T19:42:14Z

@elasticmachine merge upstream

cee-chen · 2021-06-23T21:32:52Z

@elasticmachine merge upstream

cee-chen · 2021-06-23T21:34:26Z

CI seemed to be hanging, so I gave it the ol' razzle dazzle

cee-chen · 2021-06-23T22:37:13Z

@elasticmachine merge upstream

cee-chen · 2021-06-24T02:58:31Z

@elasticmachine merge upstream

kibanamachine · 2021-06-24T05:02:44Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`enterpriseSearch`	2.1MB	2.1MB	+599.0B

History

💔 Build #133766 failed 5cbcfdd
💔 Build #133601 failed 8a1e666

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

…ic#103113) * Updated search-ui version 1.6.0 adds the ability to authenticate without a search key, which we need for this fix. * Updated Documents page to not use a search key This change updates the page to rely on basic authentication rather than a search key. As part of that, we needed to create a proxy endpoint for search-ui to post to, rather than going directly to the ent-search API. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-24T05:05:11Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

kibanamachine · 2021-06-28T05:08:07Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…) (#103204) * Updated search-ui version 1.6.0 adds the ability to authenticate without a search key, which we need for this fix. * Updated Documents page to not use a search key This change updates the page to rely on basic authentication rather than a search key. As part of that, we needed to create a proxy endpoint for search-ui to post to, rather than going directly to the ent-search API. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>

JasonStoltz added 2 commits June 23, 2021 10:42

Updated search-ui version

fb5edb3

1.6.0 adds the ability to authenticate without a search key, which we need for this fix.

Updated Documents page to not use a search key

8a1e666

This change updates the page to rely on basic authentication rather than a search key. As part of that, we needed to create a proxy endpoint for search-ui to post to, rather than going directly to the ent-search API.

JasonStoltz requested a review from a team June 23, 2021 14:59

JasonStoltz added v7.14.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 23, 2021

cee-chen reviewed Jun 23, 2021

View reviewed changes

Merge branch 'master' into fix-docs-page

5cbcfdd

cee-chen approved these changes Jun 23, 2021

View reviewed changes

Merge branch 'master' into fix-docs-page

4c4e47c

Merge branch 'master' into fix-docs-page

17a5495

Merge branch 'master' into fix-docs-page

139a87e

cee-chen enabled auto-merge (squash) June 24, 2021 03:08

cee-chen merged commit da7cdb6 into elastic:master Jun 24, 2021

kibanamachine mentioned this pull request Jun 24, 2021

[7.x] [App Search] Fixed Documents view for Editor and Analyst roles (#103113) #103204

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 28, 2021

JasonStoltz deleted the fix-docs-page branch June 28, 2021 13:41

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Fixed Documents view for Editor and Analyst roles #103113

[App Search] Fixed Documents view for Editor and Analyst roles #103113

JasonStoltz commented Jun 23, 2021

cee-chen left a comment

cee-chen Jun 23, 2021

cee-chen Jun 23, 2021

cee-chen Jun 23, 2021

cee-chen Jun 23, 2021 •

edited

Loading

cee-chen Jun 23, 2021

JasonStoltz Jun 23, 2021

cee-chen Jun 23, 2021 •

edited

Loading

cee-chen Jun 23, 2021 •

edited

Loading

JasonStoltz Jun 23, 2021

cee-chen Jun 23, 2021

cee-chen Jun 23, 2021

JasonStoltz Jun 23, 2021

cee-chen Jun 23, 2021

JasonStoltz Jun 23, 2021 •

edited

Loading

cee-chen Jun 23, 2021

JasonStoltz Jun 23, 2021

cee-chen commented Jun 23, 2021

cee-chen commented Jun 23, 2021

cee-chen commented Jun 23, 2021

cee-chen commented Jun 23, 2021

cee-chen commented Jun 24, 2021

kibanamachine commented Jun 24, 2021

kibanamachine commented Jun 24, 2021

kibanamachine commented Jun 28, 2021

		// Search UI also uses the click tracking and query suggestion endpoints, however, since the
		// App Search plugin doesn't use that portion of Search UI, we only set up a proxy for the search endpoint below.

	path: '/api/app_search/search-ui/api/as/v1/engines/{engineName}/search.json',
	path: '/api/app_search/engines/{engineName}/search-ui/search.json',

[App Search] Fixed Documents view for Editor and Analyst roles #103113

[App Search] Fixed Documents view for Editor and Analyst roles #103113

Conversation

JasonStoltz commented Jun 23, 2021

Summary

For maintainers

cee-chen left a 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

cee-chen Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen Jun 23, 2021 • edited Loading

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

JasonStoltz Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen commented Jun 23, 2021

cee-chen commented Jun 23, 2021

cee-chen commented Jun 23, 2021

cee-chen commented Jun 23, 2021

cee-chen commented Jun 24, 2021

kibanamachine commented Jun 24, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Jun 24, 2021

💚 Backport successful

kibanamachine commented Jun 28, 2021

cee-chen Jun 23, 2021 •

edited

Loading

cee-chen Jun 23, 2021 •

edited

Loading

cee-chen Jun 23, 2021 •

edited

Loading

JasonStoltz Jun 23, 2021 •

edited

Loading