Skip to content
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

[Search Session] Fix integration in vega, timelion and TSVB #87862

Merged
merged 10 commits into from
Jan 13, 2021

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Jan 11, 2021

Summary

This pr address some search session integration issues in vega, timelion, and TSVB panels.

Part of #61738
Follow up on #82835, #82232, #82229

  • Similar to visualize/lens integration pass local sessionId received from expression pipeline instead of getting it from global data service
  • Because timelion and TSVB don't use client-side data service:
    • Pass isRestore and isStored boolean flags to the server
    • Call trackSearch/untrackSearch on a session

How to test

In kibana.yml:

xpack.data_enhanced.search.sendToBackground.enabled: true
  1. Create a dashboard with vega, timelion, and TSVB panels. Use absolute time range.
  2. Save the session.
  3. Get a session id from an inspector or network request. Append it to your dashboard URL: dashboardUrl&searchSessionId={your-id}
  4. Use this URL, all panels should be loaded and the session indicator should be in "restored" state.

to make sure we are actually restoring data for all the panels try augmenting the dashboard state in the URL, e.g. changing time, or use a fake session id in the URL. All panels should fail.

For maintainers

@Dosant Dosant changed the title fix search sesion integration in vega, timelion, timeseries [Search Session] Fix integration in vega, timelion and TSVB Jan 11, 2021
@Dosant Dosant added release_note:skip Skip the PR/issue when compiling release notes Team:AppServices v7.12.0 v8.0.0 Feature:Search Querying infrastructure in Kibana labels Jan 11, 2021
@Dosant Dosant marked this pull request as ready for review January 11, 2021 21:16
@Dosant Dosant requested a review from a team January 11, 2021 21:16
@elasticmachine
Copy link
Contributor

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

@@ -93,7 +96,15 @@ export function getTimelionRequestHandler({

// parse the time range client side to make sure it behaves like other charts
const timeRangeBounds = timefilter.calculateBounds(timeRange);
const sessionId = getDataSearch().session.getSessionId();
const isCurrentSession = () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

just interesting, why you defined isCurrentSession as a function and execute it each time.

can we do something like: const isCurrentSession = !!searchSessionId && searchSessionId === dataSearch.session.getSessionId();

Copy link
Contributor Author

@Dosant Dosant Jan 12, 2021

Choose a reason for hiding this comment

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

we need to "recalculate" it in finally clause because it could already be that when async call finishes dataSearch.session.getSessionId(); has changed.
technically it won't be a problem to call untrack in that case either, but I am doing it for sanity, mostly to make it easier to debug the internal current session state by not calling untrack where we know it is not necessary

@Dosant Dosant requested a review from a team as a code owner January 12, 2021 10:06
@Dosant Dosant requested a review from alexwizp January 12, 2021 10:07
fetchMock.mockReset();
});

test('infers isRestore from session service state', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests were moved into session_service together with underlying logic.

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM in case of green CI

…sc-integrations

# Conflicts:
#	src/plugins/data/public/search/session/session_service.test.ts
#	src/plugins/data/public/search/session/session_service.ts
@Dosant Dosant requested a review from lizozom January 13, 2021 10:15
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Kibana app code changes LGTM, thanx for that @Dosant!

@Dosant
Copy link
Contributor Author

Dosant commented Jan 13, 2021

@elasticmachine merge upstream

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.

Haven't checked out.
Overall LGTM

@Dosant
Copy link
Contributor Author

Dosant commented Jan 13, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id before after diff
data 1003.6KB 1003.8KB +138.0B
dataEnhanced 39.4KB 39.4KB +39.0B
visTypeTimelion 34.8KB 36.3KB +1.5KB
visTypeTimeseries 137.0KB 137.4KB +484.0B
visTypeVega 60.5KB 60.7KB +137.0B
total +2.3KB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants