[App Search] Fixed Documents view for Editor and Analyst roles #103113
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,8 @@ | |||||
|
|
||||||
| import { schema } from '@kbn/config-schema'; | ||||||
|
|
||||||
| import { skipBodyValidation } from '../../lib/route_config_helpers'; | ||||||
|
|
||||||
| import { RouteDependencies } from '../../plugin'; | ||||||
|
|
||||||
| export function registerSearchRoutes({ | ||||||
|
|
@@ -36,4 +38,25 @@ export function registerSearchRoutes({ | |||||
| path: '/api/as/v1/engines/:engineName/search.json', | ||||||
| }) | ||||||
| ); | ||||||
|
|
||||||
| // For the Search UI routes below, Search UI always uses the full API path, like: | ||||||
| // "/api/as/v1/engines/{engineName}/search.json". We only have control over the base path | ||||||
| // in Search UI, so we created a common basepath of "/api/app_search/search-ui" here that | ||||||
| // Search UI can use. | ||||||
| // | ||||||
| // 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. | ||||||
|
Comment on lines
+47
to
+48
There was a problem hiding this comment. 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? There was a problem hiding this comment. From my own testing:
@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? There was a problem hiding this comment. 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. There was a problem hiding this comment.
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.
We should fix this, I don't think they should be logged
I think the endgame is to not log any of these queries or clicks.
I don't know anything about that.
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.
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 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. There was a problem hiding this comment.
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:
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 🤔 There was a problem hiding this comment.
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 There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 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 There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. |
||||||
| router.post( | ||||||
| skipBodyValidation({ | ||||||
| path: '/api/app_search/search-ui/api/as/v1/engines/{engineName}/search.json', | ||||||
|
There was a problem hiding this comment. 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
Instead to better match our other Kibana API endpoints? There was a problem hiding this comment. 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: 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. There was a problem hiding this comment. Ohhh gotcha gotcha. No worries in that case, my bad for failing to understand! There was a problem hiding this comment. It's not your bad, it's mine for poor writing 😢 |
||||||
| validate: { | ||||||
| params: schema.object({ | ||||||
| engineName: schema.string(), | ||||||
| }), | ||||||
| }, | ||||||
| }), | ||||||
| enterpriseSearchRequestHandler.createRequest({ | ||||||
| path: '/api/as/v1/engines/:engineName/search.json', | ||||||
| }) | ||||||
| ); | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice find on this! 💯