Skip to content

[data.search.session] Search session UI telemetry#89950

Merged
Dosant merged 28 commits intoelastic:masterfrom
lukasolson:search-session-telemetry
Feb 16, 2021
Merged

[data.search.session] Search session UI telemetry#89950
Dosant merged 28 commits intoelastic:masterfrom
lukasolson:search-session-telemetry

Conversation

@lukasolson
Copy link
Contributor

@lukasolson lukasolson commented Feb 2, 2021 •
edited
Loading

Summary

Part of #62964.

Adds telemetry for the following:

  • Session sent to background
  • Session saved after complete
  • Session restored
  • Session reloaded
  • Session extended
  • Session cancelled
  • Session deleted?

Checklist

For maintainers

@lukasolson lukasolson added Feature:Search Querying infrastructure in Kibana WIP Work in progress v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes Project:AsyncSearch Background search, partial results, async search services. v7.12.0 labels Feb 2, 2021
@lukasolson lukasolson self-assigned this Feb 2, 2021
@lukasolson lukasolson changed the title [skip-ci] [WIP] Search session telemetry [data.search.session] Search session telemetry Feb 5, 2021
@lukasolson lukasolson changed the title [data.search.session] Search session telemetry [data.search.session] Search session UI telemetry Feb 5, 2021
@lukasolson lukasolson marked this pull request as ready for review February 5, 2021 18:25
@lukasolson lukasolson requested a review from a team as a code owner February 5, 2021 18:25
@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 5, 2021

Pinging @elastic/kibana-app-services (Team:AppServices)

@lukasolson lukasolson requested review from Dosant and lizozom February 5, 2021 18:25
lizozom
lizozom reviewed Feb 8, 2021
View reviewed changes
lizozom
lizozom reviewed Feb 8, 2021
View reviewed changes
lizozom
lizozom approved these changes Feb 10, 2021
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM
I would however add another event upon arriving to the management app

lizozom
lizozom reviewed Feb 11, 2021
View reviewed changes
x-pack/plugins/data_enhanced/public/search/sessions_mgmt/components/table/table.tsx
useEffect(() => {
doRefresh();
}, [doRefresh]);
plugins.data.search.usageCollector?.trackSessionsListLoaded();
Copy link
Contributor

@lizozom lizozom Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might be able to replace this by using the sub view collector for the management app. Not sure we want to tho 🤔 https://github.com/elastic/kibana/blob/master/src/plugins/kibana_usage_collection/server/collectors/application_usage/README.md

@lizozom
Copy link
Contributor

lizozom commented Feb 15, 2021

@elasticmachine merge upstream
@elasticmachine merge upstream

Dosant
Dosant approved these changes Feb 15, 2021
View reviewed changes
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks goods.
Two things I'd improved:

...ns/data_enhanced/public/search/ui/connected_search_session_indicator/search_session_tour.tsx Outdated
}

if (state === SearchSessionState.Restored) {
usageCollector?.trackSessionIsRestored();
Copy link
Contributor

@Dosant Dosant Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not the best place to put it because it is part of the tour hook.
It would make more sense to have in inside the x-pack/plugins/data_enhanced/public/search/ui/connected_search_session_indicator/connected_search_session_indicator.tsx or session_service itself (when restore) is called

Copy link
Contributor

@Dosant Dosant Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in: lukasolson#18

@Dosant Dosant mentioned this pull request Feb 15, 2021
Merged
Dosant
Dosant reviewed Feb 15, 2021
View reviewed changes
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some improvements:
lukasolson#18

@lukasolson, @lizozom, could you please take a look?
Those also are not critical and we can merge this pr first and direct lukasolson#18 as a bugfix to master.

src/plugins/data/public/search/collectors/types.ts Outdated
/**
* The session indicator was disabled because of a completion timeout
*/
SESSION_INDICATOR_TOUR_DISABLED = 'sessionIndicatorTourDisabled',
Copy link
Contributor

@Dosant Dosant Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this to SESSION_INDICATOR_SAVE_DISABLED in lukasolson#18

...ns/data_enhanced/public/search/ui/connected_search_session_indicator/search_session_tour.tsx Outdated
if (state === SearchSessionState.Restored) {
usageCollector?.trackSessionIsRestored();
if (!safeHas(storage, TOUR_RESTORE_STEP_KEY)) {
usageCollector?.trackSessionIndicatorTourRestored();
Copy link
Contributor

@Dosant Dosant Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call here is not reliable and can be called multiple times 😮
Because of how this useEffect with searchSessionIndicatorRef is setup 😢

This probably won't happen in real life, but I noticed it in new unit tests I added here: lukasolson#18

So I put up a fix to make sure we 100% won't cause excessive tracks: lukasolson#18

@Dosant
Copy link
Contributor

Dosant commented Feb 15, 2021

@elasticmachine merge upstream

@Dosant
Copy link
Contributor

Dosant commented Feb 16, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

kibanamachine commented Feb 16, 2021

💚 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
dataEnhanced 160.4KB 161.7KB +1.3KB

Page load bundle

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

id before after diff
data 904.6KB 907.0KB +2.4KB
dataEnhanced 38.7KB 40.2KB +1.6KB
total +3.9KB

History

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

@Dosant Dosant merged commit f06e722 into elastic:master Feb 16, 2021
Dosant added a commit to Dosant/kibana that referenced this pull request Feb 16, 2021
@lukasolson @Dosant
[Search Sessions] Search session UI telemetry (elastic#89950)
53df7a9 
Co-authored-by: Liza K <liza.katz@elastic.co>
Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
@Dosant Dosant mentioned this pull request Feb 16, 2021
Merged
Dosant added a commit that referenced this pull request Feb 16, 2021
@Dosant @lukasolson
[Search Sessions] Search session UI telemetry (#89950) (#91461)
a19844c 
Co-authored-by: Liza K <liza.katz@elastic.co>
Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Dosant Dosant Dosant approved these changes

+1 more reviewer

@lizozom lizozom lizozom approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lukasolson lukasolson

Labels

Feature:Search Querying infrastructure in Kibana Project:AsyncSearch Background search, partial results, async search services. release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0 WIP Work in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lukasolson @elasticmachine @lizozom @Dosant @kibanamachine