[Search] Fix telemetry collection to not send extra requests #73382
Conversation
lukasolson
added
review
Feature:Search
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
| @@ -71,7 +70,7 @@ export function usageProvider(core: CoreSetup<object, DataPluginStart>): SearchU | |||
| }; | |||
|
|
|||
| return { | |||
| trackError: getTracker('errorCount'), | |||
| trackError: () => getTracker('errorCount')(), | |||
There was a problem hiding this comment.
Why is the cide different for the error and the success handlers?
There was a problem hiding this comment.
We don't really have an accurate way to see the time a failure took since there is no took in the response for a failure. So this was just to make typescript happy since the error handler for errorCount didn't accept a duration.
|
|
||
| const newAttributes = { ...attributes, averageDuration }; | ||
| // Only track the average duration for successful requests | ||
| if (eventType === 'successCount') { |
There was a problem hiding this comment.
I actually think we should track the time to a failure too (an error status code or something for failures is important too)
| @@ -30,9 +31,14 @@ export interface AsyncSearchResponse<T> { | |||
| response: SearchResponse<T>; | |||
| } | |||
|
|
|||
| export function isAsyncSearchResponse<T>(response: any): response is AsyncSearchResponse<T> { | |||
There was a problem hiding this comment.
(a) instead of any you could use the response base class.
(b) no need to export this function as well as AsyncSearchResponse - it's not used (and shouldn't be used) externally.
There was a problem hiding this comment.
👍 I'll give this a try
| usage && | ||
| (!isAsyncSearchResponse<any>(response) || (!response.is_partial && !response.is_running)) | ||
| ) { | ||
| usage.trackSuccess(response.rawResponse.took); |
There was a problem hiding this comment.
You also need to add an else clause with
if (response.is_partial && !response.is_running) {
usage.trackError();
}
And adding some information to that failure would be great.
There was a problem hiding this comment.
Agreed, we'll have to add additional metadata in a subsequent PR but I cant add this error case.
💚 Build SucceededBuild metricspage load bundle size
History
To update your PR or re-run it, just comment with: |
…#73382) * [Search] Fix telemetry collection to not send extra requests * Update tracker to collect duration from ES response * Fix type check * Update docs and types * Fix typescript Co-authored-by: Liza K <liza.katz@elastic.co>
…#73382) * [Search] Fix telemetry collection to not send extra requests * Update tracker to collect duration from ES response * Fix type check * Update docs and types * Fix typescript Co-authored-by: Liza K <liza.katz@elastic.co>
…#74156) * [Search] Fix telemetry collection to not send extra requests * Update tracker to collect duration from ES response * Fix type check * Update docs and types * Fix typescript Co-authored-by: Liza K <liza.katz@elastic.co> Co-authored-by: Liza K <liza.katz@elastic.co>
…#74158) * [Search] Fix telemetry collection to not send extra requests * Update tracker to collect duration from ES response * Fix type check * Update docs and types * Fix typescript Co-authored-by: Liza K <liza.katz@elastic.co> Co-authored-by: Liza K <liza.katz@elastic.co>
Summary
Fixes #73116.
This PR updates the usage collection for search to not send an additional request from the browser for each search request. Instead, we collect the telemetry from the server where we update the saved object that is used for telemetry.