-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Report telemetry for autocomplete #91428
Report telemetry for autocomplete #91428
Conversation
* error cound * position of selected suggestion * length of query used to generate the suggestions Also added a couple if handly telemetry events: * query language switch * filter bar bulk actions
…autocomplete-telemetry
Pinging @elastic/kibana-app-services (Team:AppServices) |
src/plugins/data/public/autocomplete/providers/value_suggestion_provider.ts
Show resolved
Hide resolved
if (props.trackUiMetric) { | ||
props.trackUiMetric(METRIC_TYPE.CLICK, `${props.appName}:filter_invertInclusion`); | ||
} | ||
reportUiCounter?.(METRIC_TYPE.CLICK, `filter:invert_all`); |
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.
Does this change how we report existing metrics? Isn't it breaking as it would make those harder to analyze?
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.
We never analyze them anyway...
I thought it was good timing for some standardization
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.
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.
@majagrubic would you mind taking a look?
I wanted to align the names of telemetry events. Will this break anything for you?
Thanks!
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.
Is my understanding correct here that we're no longer tracking the appName
with the metric? This would make it impossible for us to track which clicks came from inside Discover for example?
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.
As I understood, we get an appname
thanks to the binding right here:
https://github.com/elastic/kibana/pull/91428/files#diff-b370985f7060e1ee97121c246b0ac878bf5b1dee2271e0593a983f893141109eR47
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.
@majagrubic We still report the appName
, I just renamed some of the events themselves and it should be easier to analyze as the appName now is in its own field
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.
I think it's fine with us, we're not actively using these metrics yet.
src/plugins/data/public/autocomplete/collectors/create_usage_collector.ts
Show resolved
Hide resolved
@elasticmachine merge upstream |
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.
LGTM
return await requestSuggestions(title, field, query, filters, signal); | ||
} catch (e) { | ||
if (!signal?.aborted) { | ||
usageCollector?.trackError(); |
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.
nit: I think this also belongs to requestSuggestions
and should be similar to usageCollector?.trackResult();
call
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.
I agree, but I didn't want to delete anything from requestSugestions.cache
before the function returned.
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* Report telemetry about autocomplete * error cound * position of selected suggestion * length of query used to generate the suggestions Also added a couple if handly telemetry events: * query language switch * filter bar bulk actions * Fix ts * Report the value suggestions funnel * docs * docs * fix jest * test * code review Co-authored-by: Kibana Machine <[email protected]>
* Report telemetry about autocomplete * error cound * position of selected suggestion * length of query used to generate the suggestions Also added a couple if handly telemetry events: * query language switch * filter bar bulk actions * Fix ts * Report the value suggestions funnel * docs * docs * fix jest * test * code review Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Added some telemetry around the data plugin
Autocomplete interactions
This should allow us to analyze how often and how well users engage with our existing Autocomplete results.
Value suggestions requests
Other UI components
Checklist
Delete any items that are not applicable to this PR.
For maintainers