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

[RFC][Search] Background sessions RFC #73281

Merged
merged 34 commits into from
Oct 5, 2020
Merged

[RFC][Search] Background sessions RFC #73281

merged 34 commits into from
Oct 5, 2020

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Jul 27, 2020

Summary

RFC for the Background Sessions feature in Kibana

Replaces #69117

@lizozom lizozom added Meta v8.0.0 release_note:skip Skip the PR/issue when compiling release notes RFC v7.10.0 Feature:Search Querying infrastructure in Kibana labels Jul 27, 2020
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
```

Retrieves all of the background session saved objects for the current user. (We can filter it to the current user
because when the saved object is created, we store a hash of the current user's username inside the object.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just the username isn't sufficient for these to be "per user"... @elastic/kibana-security if they were to store the username AND the realm, would that be sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

@elastic/kibana-security if they were to store the username AND the realm, would that be sufficient?

The most unique thing we can get right now is username:realm-name:realm-type (or username:auth-provider-name:auth-provider-type). There is a risk that realm with the same name and type will be reconfigured to work with another IdP/user-store, but we consider this risk as low.

Copy link
Member

Choose a reason for hiding this comment

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

I know there is an API to get the current username, but is there a client-side API to get the realm and realm type?

rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
@timroes
Copy link
Contributor

timroes commented Aug 6, 2020

From your chart in the architecture review I understood, that we have a cache (before sending something to background) mapping the request hash and the session id to the actual ES async id. And this cache entry will be stored in a saved object when we're sending it to background. When will an entry be removed from that cache? When it finished loading?

If we remove that when finished loading (or also in general) I'd be curious what happens in this scenario:

We load a dashboard and send it to background while some of the visualizations are already loaded and others are still loading. Will those finished requests of the visualization that have loaded already also been "send to background"? If not, won't all those visualizations that were already loaded when we send to background, fail as soon as we later watch the "all background queries loaded" dashboard, since they will create a cache miss (which fails the request as you explained yesterday). How are those visualizations adressed that are already loaded when we send to background, so they'll still work after we send to background and watch the dashboard later when we get the notification that it's ready?

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

provided some initial comments/questions

rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
rfcs/text/0012_background_sessions.md Outdated Show resolved Hide resolved
### New Session Service

```ts
interface ISessionService {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the server and client new session service have same interface ?

Copy link
Member

Choose a reason for hiding this comment

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

++ IMO we should keep these the same unless there's some reason not to.

```ts
interface SearchService {
/**
* The search API will accept the option `trackId`, which will track the search ID, if available, on the server, until a corresponding saved object is created.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see trackId here, but I'm guessing it would be in ISearchOptions?

### New Session Service

```ts
interface ISessionService {
Copy link
Member

Choose a reason for hiding this comment

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

++ IMO we should keep these the same unless there's some reason not to.

* @returns a filtered list of BackgroundSessionAttributes objects.
* @throws Throws an error in OSS.
*/
list: (options: SavedObjectsFindOptions) => BackgroundSessionAttributes[]
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 supposed to be async?

Suggested change
list: (options: SavedObjectsFindOptions) => BackgroundSessionAttributes[]
list: (options: SavedObjectsFindOptions) => Promise<BackgroundSessionAttributes[]>

rfcs/text/0013_background_sessions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

This is looking great!


### Feature Controls

Background sessions as a feature will be enabled/disabled per role/space by an admin. When set to "all", the feature will be available in its entirety, and when set to "read" or "none", the feature will be unavailable (i.e. search requests will only continue to run while a user waits on page, with no way to continue requests in the background).
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding Background sessions as a top-level feature is rather awkward. Top-level features are generally applications that show up in the side-bar or Kibana specific management sections. As such, after #78152 merges, these will be grouped on the Spaces management screen and the Role management screen:

Screen Shot 2020-09-29 at 12 39 57 PM

Screen Shot 2020-09-29 at 12 41 12 PM

In my opinion, Background Sessions aren't really a Kibana, Observability, Security or Management feature.

We could potentially model background sessions as a sub-feature of the existing features. For example, this is how this would look for the Dashboard feature:

Screen Shot 2020-09-29 at 12 43 22 PM

This avoids the awkwardness of the following:

when set to "read" or "none", the feature will be unavailable

It's entirely possible that I'm mis-understanding what we're trying to achieve here. Do we want to have background sessions available to some users but not others? Or are we just trying to provide a mechanism for users to turn off background sessions, if they don't want to incur the overhead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do want BGS to be available to specific users.
However, I don't think that enabling them per application would be a logical way to enable them.
It's a cross application feature, like telemetry or notifications for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want existing users who only have all/read access to features like Dashboards to be able to create background sessions?


### Saved Object Structure

The saved object created for a background session will be scoped to a single space, and will be a `hidden` saved object
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're only concerned with these saved-objects showing up in the management listing, you don't have to make them hidden: true, you can just not declare the management key.

However, I do think that we'll want these to be hidden saved-objects for another reason. Is my understanding correct that we only want the user that created a background session to be able to CRUD their own background sessions? As far as I'm aware, this is only possible currently by declaring the saved-object as being hidden and writing a custom "Client" which performs its own authorization and filtering of these saved-objects.

Copy link
Member

Choose a reason for hiding this comment

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

However, I do think that we'll want these to be hidden saved-objects for another reason. Is my understanding correct that we only want the user that created a background session to be able to CRUD their own background sessions? As far as I'm aware, this is only possible currently by declaring the saved-object as being hidden and writing a custom "Client" which performs its own authorization and filtering of these saved-objects.

++ that's absolutely right. We discussed this here during an earlier iteration of this RFC

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's correct. We don't want these saved objects showing up in the general saved object management listing. We will have a separate, background-session-specific management view. The list will be filtered based on the userId described below, and will be managed through the expire/extend/etc. APIs described below.

@lizozom lizozom added the RFC/final-comment-period If no concerns are raised in 3 business days this RFC will be accepted label Sep 30, 2020
@lizozom
Copy link
Contributor Author

lizozom commented Sep 30, 2020

Thanks everyone for your reviews and suggestions!
The RFC has just been moved into its final comment period and it will be merged on Monday, unless no critical input is added.
🙇‍♀️

@kobelb
Copy link
Contributor

kobelb commented Sep 30, 2020

After chatting with @lukasolson, the feature controls section has been removed from the RFC. It's still a discussion we need to have, and a decision should be made on how to integrate this with feature controls, sooner rather than later especially if it requires changes to our authorization model. However, RFCs won't ever specify 100% of the details of the final implementation, so we'll deal with this complication out-of-band of the RFC.

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@lizozom lizozom merged commit 5f53d4f into elastic:master Oct 5, 2020
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 7, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 73281 or prevent reminders by adding the backport:skip label.

@lukasolson lukasolson added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Search Querying infrastructure in Kibana Meta release_note:skip Skip the PR/issue when compiling release notes RFC/final-comment-period If no concerns are raised in 3 business days this RFC will be accepted RFC v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants