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] Use session service on a dashboard #81297

Merged
merged 18 commits into from
Oct 29, 2020
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ export declare type EmbeddableInput = {
timeRange?: TimeRange;
query?: Query;
filters?: Filter[];
searchSessionId?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a major nit, but do we really want to have the name be so specific?
I think sessions can and will be used for more than search.

Copy link
Contributor Author

@Dosant Dosant Oct 29, 2020

Choose a reason for hiding this comment

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

I think sessionId is way too vague for dashboards / embeddable level, but OK on search level.
sessionId on dashboard could mean anything:

  • user authentication session
  • dashboard page session (created once when user loaded a dashboard)
  • dashboard app session (to track drill-downs between dashboards without navigating away from a dashboard app)
  • and the search / fetch session.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, if this terminology is for using in solutions :)

};
```
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export class DashboardAppController {
dashboardCapabilities,
scopedHistory,
embeddableCapabilities: { visualizeCapabilities, mapsCapabilities },
data: { query: queryService },
data: { query: queryService, search: searchService },
core: {
notifications,
overlays,
Expand Down Expand Up @@ -412,8 +412,9 @@ export class DashboardAppController {
>(DASHBOARD_CONTAINER_TYPE);

if (dashboardFactory) {
const searchSessionId = searchService.session.start();
dashboardFactory
.create(getDashboardInput())
.create({ ...getDashboardInput(), searchSessionId })
.then((container: DashboardContainer | ErrorEmbeddable | undefined) => {
if (container && !isErrorEmbeddable(container)) {
dashboardContainer = container;
Expand Down Expand Up @@ -572,7 +573,7 @@ export class DashboardAppController {
differences.filters = appStateDashboardInput.filters;
}

Object.keys(_.omit(containerInput, ['filters'])).forEach((key) => {
Object.keys(_.omit(containerInput, ['filters', 'searchSessionId'])).forEach((key) => {
const containerValue = (containerInput as { [key: string]: unknown })[key];
const appStateValue = ((appStateDashboardInput as unknown) as { [key: string]: unknown })[
key
Expand All @@ -590,7 +591,8 @@ export class DashboardAppController {
const refreshDashboardContainer = () => {
const changes = getChangesFromAppStateForContainerState();
if (changes && dashboardContainer) {
dashboardContainer.updateInput(changes);
const searchSessionId = searchService.session.start();
dashboardContainer.updateInput({ ...changes, searchSessionId });
}
};

Expand Down Expand Up @@ -1109,12 +1111,6 @@ export class DashboardAppController {
$scope.model.filters = filterManager.getFilters();
$scope.model.query = queryStringManager.getQuery();
dashboardStateManager.applyFilters($scope.model.query, $scope.model.filters);
if (dashboardContainer) {
dashboardContainer.updateInput({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed. After removing all dashboard container updates are handled in single place right now.
Looking forward to this file being refactored.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in the middle of completely re-doing this file as part of deangularization. It will be a functional component - I may end up pinging you for some input at some point.

Copy link
Contributor Author

@Dosant Dosant Oct 27, 2020

Choose a reason for hiding this comment

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

@ThomThomson, that's great news 👍 Will be glad to chime in.
From perspective of this pr important bit is that we are calling dashboardContainer.updateInput in a single place and we assume this is when we want to start a new search session.

filters: $scope.model.filters,
query: $scope.model.query,
});
}
},
});

Expand Down Expand Up @@ -1159,6 +1155,7 @@ export class DashboardAppController {
if (dashboardContainer) {
dashboardContainer.destroy();
}
searchService.session.clear();
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,25 @@ test('Container view mode change propagates to new children', async () => {

expect(embeddable.getInput().viewMode).toBe(ViewMode.EDIT);
});

test('searchSessionId propagates to children', async () => {
const searchSessionId1 = 'searchSessionId1';
const container = new DashboardContainer(
getSampleDashboardInput({ searchSessionId: searchSessionId1 }),
options
);
const embeddable = await container.addNewEmbeddable<
ContactCardEmbeddableInput,
ContactCardEmbeddableOutput,
ContactCardEmbeddable
>(CONTACT_CARD_EMBEDDABLE, {
firstName: 'Bob',
});

expect(embeddable.getInput().searchSessionId).toBe(searchSessionId1);

const searchSessionId2 = 'searchSessionId2';
container.updateInput({ searchSessionId: searchSessionId2 });

expect(embeddable.getInput().searchSessionId).toBe(searchSessionId2);
});
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export interface InheritedChildInput extends IndexSignature {
viewMode: ViewMode;
hidePanelTitles?: boolean;
id: string;
searchSessionId?: string;
}

export interface DashboardContainerOptions {
Expand Down Expand Up @@ -228,7 +229,15 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
}

protected getInheritedInput(id: string): InheritedChildInput {
const { viewMode, refreshConfig, timeRange, query, hidePanelTitles, filters } = this.input;
const {
viewMode,
refreshConfig,
timeRange,
query,
hidePanelTitles,
filters,
searchSessionId,
} = this.input;
return {
filters,
hidePanelTitles,
Expand All @@ -237,6 +246,7 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
refreshConfig,
viewMode,
id,
searchSessionId,
};
}
}
7 changes: 6 additions & 1 deletion src/plugins/data/public/search/expressions/esaggs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export interface RequestHandlerParams {
metricsAtAllLevels?: boolean;
visParams?: any;
abortSignal?: AbortSignal;
searchSessionId?: string;
}

const name = 'esaggs';
Expand All @@ -82,6 +83,7 @@ const handleCourierRequest = async ({
inspectorAdapters,
filterManager,
abortSignal,
searchSessionId,
}: RequestHandlerParams) => {
// Create a new search source that inherits the original search source
// but has the appropriate timeRange applied via a filter.
Expand Down Expand Up @@ -143,13 +145,15 @@ const handleCourierRequest = async ({
defaultMessage:
'This request queries Elasticsearch to fetch the data for the visualization.',
}),
searchSessionId,
}
);
request.stats(getRequestInspectorStats(requestSearchSource));

try {
const response = await requestSearchSource.fetch({
abortSignal,
sessionId: searchSessionId,
});

request.stats(getResponseInspectorStats(response, searchSource)).ok({ json: response });
Expand Down Expand Up @@ -248,7 +252,7 @@ export const esaggs = (): EsaggsExpressionFunctionDefinition => ({
multi: true,
},
},
async fn(input, args, { inspectorAdapters, abortSignal }) {
async fn(input, args, { inspectorAdapters, abortSignal, getSearchSessionId }) {
const indexPatterns = getIndexPatterns();
const { filterManager } = getQueryService();
const searchService = getSearchService();
Expand Down Expand Up @@ -276,6 +280,7 @@ export const esaggs = (): EsaggsExpressionFunctionDefinition => ({
inspectorAdapters: inspectorAdapters as Adapters,
filterManager,
abortSignal: (abortSignal as unknown) as AbortSignal,
searchSessionId: getSearchSessionId(),
});

const table: Datatable = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export class SearchEmbeddable
private prevTimeRange?: TimeRange;
private prevFilters?: Filter[];
private prevQuery?: Query;
private prevSearchSessionId?: string;

constructor(
{
Expand Down Expand Up @@ -266,6 +267,8 @@ export class SearchEmbeddable
}

private fetch = async () => {
const searchSessionId = this.input.searchSessionId;

if (!this.searchScope) return;

const { searchSource } = this.savedSearch;
Expand All @@ -292,7 +295,11 @@ export class SearchEmbeddable
const description = i18n.translate('discover.embeddable.inspectorRequestDescription', {
defaultMessage: 'This request queries Elasticsearch to fetch the data for the search.',
});
const inspectorRequest = this.inspectorAdaptors.requests.start(title, { description });

const inspectorRequest = this.inspectorAdaptors.requests.start(title, {
description,
searchSessionId,
});
inspectorRequest.stats(getRequestInspectorStats(searchSource));
searchSource.getSearchRequestBody().then((body: Record<string, unknown>) => {
inspectorRequest.json(body);
Expand All @@ -303,6 +310,7 @@ export class SearchEmbeddable
// Make the request
const resp = await searchSource.fetch({
abortSignal: this.abortController.signal,
sessionId: searchSessionId,
});
this.updateOutput({ loading: false, error: undefined });

Expand All @@ -324,7 +332,8 @@ export class SearchEmbeddable
!esFilters.onlyDisabledFiltersChanged(this.input.filters, this.prevFilters) ||
!_.isEqual(this.prevQuery, this.input.query) ||
!_.isEqual(this.prevTimeRange, this.input.timeRange) ||
!_.isEqual(searchScope.sort, this.input.sort || this.savedSearch.sort);
!_.isEqual(searchScope.sort, this.input.sort || this.savedSearch.sort) ||
this.prevSearchSessionId !== this.input.searchSessionId;

// If there is column or sort data on the panel, that means the original columns or sort settings have
// been overridden in a dashboard.
Expand All @@ -341,6 +350,7 @@ export class SearchEmbeddable
this.prevFilters = this.input.filters;
this.prevQuery = this.input.query;
this.prevTimeRange = this.input.timeRange;
this.prevSearchSessionId = this.input.searchSessionId;
} else if (this.searchScope) {
// trigger a digest cycle to make sure non-fetch relevant changes are propagated
this.searchScope.$applyAsync();
Expand Down
5 changes: 5 additions & 0 deletions src/plugins/embeddable/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,9 @@ export type EmbeddableInput = {
* Visualization filters used to narrow down results.
*/
filters?: Filter[];

/**
* Search session id to group searches
*/
searchSessionId?: string;
};
1 change: 1 addition & 0 deletions src/plugins/embeddable/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ export type EmbeddableInput = {
timeRange?: TimeRange;
query?: Query;
filters?: Filter[];
searchSessionId?: string;
};

// Warning: (ae-missing-release-tag) "EmbeddableInstanceConfiguration" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
Expand Down
1 change: 1 addition & 0 deletions src/plugins/inspector/common/adapters/request/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export interface Request extends RequestParams {
export interface RequestParams {
id?: string;
description?: string;
searchSessionId?: string;
}

export interface RequestStatistics {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,21 @@ export class RequestsViewComponent extends Component<InspectorViewProps, Request
</EuiText>
)}

{this.state.request && this.state.request.searchSessionId && (
Copy link
Member

Choose a reason for hiding this comment

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

Is this only for the functional tests, or do you think there is value in showing this parameter to the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would also be useful for debugging and support purposes

<EuiText size="xs">
<p
data-test-subj={'inspectorRequestSearchSessionId'}
data-search-session-id={this.state.request.searchSessionId}
>
<FormattedMessage
id="inspector.requests.searchSessionId"
defaultMessage="Search session id: {searchSessionId}"
values={{ searchSessionId: this.state.request.searchSessionId }}
/>
</p>
</EuiText>
)}

<EuiSpacer size="m" />

{this.state.request && <RequestDetails request={this.state.request} />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ export class VisualizeEmbeddable
private timeRange?: TimeRange;
private query?: Query;
private filters?: Filter[];
private searchSessionId?: string;
private visCustomizations?: Pick<VisualizeInput, 'vis' | 'table'>;
private subscriptions: Subscription[] = [];
private expression: string = '';
Expand Down Expand Up @@ -243,6 +244,11 @@ export class VisualizeEmbeddable
dirty = true;
}

if (this.searchSessionId !== this.input.searchSessionId) {
this.searchSessionId = this.input.searchSessionId;
dirty = true;
}

if (this.vis.description && this.domNode) {
this.domNode.setAttribute('data-description', this.vis.description);
}
Expand Down Expand Up @@ -374,6 +380,7 @@ export class VisualizeEmbeddable
query: this.input.query,
filters: this.input.filters,
},
searchSessionId: this.searchSessionId,
uiState: this.vis.uiState,
inspectorAdapters: this.inspectorAdapters,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ describe('embeddable', () => {
timeRange,
query,
filters,
searchSessionId: 'searchSessionId',
});

expect(expressionRenderer).toHaveBeenCalledTimes(2);
Expand All @@ -182,7 +183,13 @@ describe('embeddable', () => {
const query: Query = { language: 'kquery', query: '' };
const filters: Filter[] = [{ meta: { alias: 'test', negate: false, disabled: false } }];

const input = { savedObjectId: '123', timeRange, query, filters } as LensEmbeddableInput;
const input = {
savedObjectId: '123',
timeRange,
query,
filters,
searchSessionId: 'searchSessionId',
} as LensEmbeddableInput;

const embeddable = new Embeddable(
{
Expand Down Expand Up @@ -214,6 +221,8 @@ describe('embeddable', () => {
filters,
})
);

expect(expressionRenderer.mock.calls[0][0].searchSessionId).toBe(input.searchSessionId);
});

it('should merge external context with query and filters of the saved object', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export class Embeddable
timeRange?: TimeRange;
query?: Query;
filters?: Filter[];
searchSessionId?: string;
lastReloadRequestTime?: number;
} = {};

Expand Down Expand Up @@ -149,12 +150,14 @@ export class Embeddable
if (
!_.isEqual(containerState.timeRange, this.externalSearchContext.timeRange) ||
!_.isEqual(containerState.query, this.externalSearchContext.query) ||
!_.isEqual(cleanedFilters, this.externalSearchContext.filters)
!_.isEqual(cleanedFilters, this.externalSearchContext.filters) ||
containerState.searchSessionId !== this.externalSearchContext.searchSessionId
) {
this.externalSearchContext = {
timeRange: containerState.timeRange,
query: containerState.query,
lastReloadRequestTime: this.externalSearchContext.lastReloadRequestTime,
searchSessionId: containerState.searchSessionId,
filters: cleanedFilters,
};

Expand All @@ -177,6 +180,7 @@ export class Embeddable
ExpressionRenderer={this.expressionRenderer}
expression={this.expression || null}
searchContext={this.getMergedSearchContext()}
searchSessionId={this.externalSearchContext.searchSessionId}
handleEvent={this.handleEvent}
/>,
domNode
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface ExpressionWrapperProps {
ExpressionRenderer: ReactExpressionRendererType;
expression: string | null;
searchContext: ExecutionContextSearch;
searchSessionId?: string;
handleEvent: (event: ExpressionRendererEvent) => void;
}

Expand All @@ -27,6 +28,7 @@ export function ExpressionWrapper({
expression,
searchContext,
handleEvent,
searchSessionId,
}: ExpressionWrapperProps) {
return (
<I18nProvider>
Expand All @@ -51,6 +53,7 @@ export function ExpressionWrapper({
padding="m"
expression={expression}
searchContext={searchContext}
searchSessionId={searchSessionId}
renderError={(errorMessage, error) => (
<div data-test-subj="expression-renderer-error">
<EuiFlexGroup direction="column" alignItems="center" justifyContent="center">
Expand Down
Loading