[Feedback Needed][Search] Use session service on a dashboard #81192
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dosant
changed the title
Dosant
commented
Oct 20, 2020
| @@ -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({ | |||
There was a problem hiding this comment.
This is not needed and handled by refreshDashboardContainer. Looking forward to this file being refactored.
If keep it as is, the it would create redundant session ids.
Dosant
commented
Oct 20, 2020
| @@ -590,7 +591,8 @@ export class DashboardAppController { | |||
| const refreshDashboardContainer = () => { | |||
| const changes = getChangesFromAppStateForContainerState(); | |||
| if (changes && dashboardContainer) { | |||
| dashboardContainer.updateInput(changes); | |||
| const sessionId = searchService.session.start(); | |||
| dashboardContainer.updateInput({ ...changes, searchSessionId: sessionId }); | |||
There was a problem hiding this comment.
@lizozom, @lukasolson, @ppisljar, I am passing the sessionId down the pipeline (#76889 (comment)). Is this the correct approach or do we want sessionService to be accessed somewhere down the pipe directly?
If this is the preferred approach, then what is the need for global sessionService ?
There was a problem hiding this comment.
Ideally, we wouldn't need the global sessionService state. We'd just be passing the sessionId around. However, I think the intent of the global state was in case we didn't have the necessary pipeline to send the sessionId all the way from the application, through embeddables and expression service, to the search service. @lizozom Correct me if I'm wrong.
There was a problem hiding this comment.
if i remember correctly initially we were discussing accessing the session directly but since we figured we will have searches which we don't want to be part of the session, so we'll need to be passing them down
Dosant
changed the title
💔 Build Failed
Failed CI StepsTest FailuresChrome UI Functional Tests.test/functional/apps/management/_test_huge_fields·js.management test large number of fields "before all" hook for "test_huge data should have expected number of fields"Standard OutStack TraceMetrics [docs]async chunks size
page load bundle size
To update your PR or re-run it, just comment with: |
lukasolson
reviewed
Oct 20, 2020
| @@ -590,7 +591,8 @@ export class DashboardAppController { | |||
| const refreshDashboardContainer = () => { | |||
| const changes = getChangesFromAppStateForContainerState(); | |||
| if (changes && dashboardContainer) { | |||
| dashboardContainer.updateInput(changes); | |||
| const sessionId = searchService.session.start(); | |||
| dashboardContainer.updateInput({ ...changes, searchSessionId: sessionId }); | |||
There was a problem hiding this comment.
Ideally, we wouldn't need the global sessionService state. We'd just be passing the sessionId around. However, I think the intent of the global state was in case we didn't have the necessary pipeline to send the sessionId all the way from the application, through embeddables and expression service, to the search service. @lizozom Correct me if I'm wrong.
| @@ -84,4 +84,5 @@ export interface ExecutionContextSearch { | |||
| filters?: Filter[]; | |||
| query?: Query | Query[]; | |||
| timeRange?: TimeRange; | |||
| sessionId?: string; | |||
There was a problem hiding this comment.
I think it's probably best if we stick to searchSessionId here and below. I think it's more explicit.
| @@ -248,7 +252,7 @@ export const esaggs = (): EsaggsExpressionFunctionDefinition => ({ | |||
| multi: true, | |||
| }, | |||
| }, | |||
| async fn(input, args, { inspectorAdapters, abortSignal }) { | |||
| async fn(input, args, { inspectorAdapters, abortSignal, search }) { | |||
There was a problem hiding this comment.
Does it make sense to add this as part of args instead of handlers? Or is it even feasible?
I guess I'm not even sure how this is being passed in... Was search being passed in prior to this PR inside the third argument here?
There was a problem hiding this comment.
it will need to be a handler, i wouldn't include it as part of initial input however to make it more explicit and would prefer a separate getter function on the handlers
ppisljar
reviewed
Oct 21, 2020
| /** | ||
| * Search session id to group searches | ||
| */ | ||
| searchSessionId?: string; |
|
Closing in favour of #81297 (wip). |
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Follow up on #76889
Uses session service on a dashboard.
Notes:
sessionIddisplayed in inspector. Plan to use it in a functional test.Checklist
Delete any items that are not applicable to this PR.
For maintainers