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

[Security assistant] Conversation pagination patch #196782

Closed
wants to merge 19 commits into from

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Oct 17, 2024

Summary

Users with over 20 conversations will experience this error:

b4

In the long term we need to implement pagination for the Assistant. Unfortunately, much of our conversation update logic on the frontend requires fetching all conversations. While a larger refactor is needed, this patch increases the fetch limit from 20 to 5,000 conversations as a temporary fix.

Why 5,000? Well... The Elasticsearch max results per query is 10,000, but that seems like overkill. At first I thought 1,000, but with how small the conversation object is I tested 5,000 and everything worked as expected. Have an argument for another number? I'm open to it.

UI query optimizations made to support 5,000 conversations

  1. useFetchCurrentUserConversations Hook:
    Updated to accept fields and filter arguments. We're now retrieving only 3-4 fields per conversation and excluding the messages field to reduce data load.

  2. useAssistantOverlay Update:
    Previously, the hook fetched all conversations and mapped them to check for a title match. Now, it filters by title: ${title}, retrieving only the relevant conversation instead of fetching up to 5000.

  3. Optimization in security_solution/public/assistant/provider.tsx:
    Instead of fetching all conversations to check if any exist, getUserConversations was replaced with getUserConversationsExist, optimizing the query to return a count only, reducing unnecessary data fetching.

  4. Remove conversations fetch in in security_solution/public/assistant/stack_management/management_settings.tsx:
    I discovered a call to fetch all conversations only to locate the Welcome conversation, which was then passed to ConversationSettingsManagement in kbn-elastic-assistant, where all conversations were fetched again. This redundant call was removed, and the export from kbn-elastic-assistant was eliminated, as the references are now entirely internal.

Extra protection against Welcome conversation exists error

In the case the user has more than 5000 conversations, to prevent the "Welcome conversation exists" error and similar issues, I've added a sortField of is_default. This ensures that default conversations are returned first, followed by a secondary sort on updated_at. In the conversation selector side nav, conversations remain sorted by updated_at.

Previously, conversation.isDefault could be undefined, but it now defaults to false. This change ensures that conversations without isDefault are sorted correctly—without this, new conversations could be pushed to the bottom, even if they were recently updated. We rely on the updated_at field to display the most recent conversations at the top.

I did not modify the Elasticsearch type to require is_default, as legacy conversations won’t have this field. Instead, the value is defaulted to false in the API.

Testing

Start by creating one new conversation in the UI. Update the conversation name to OLD CONVO. Verify it shows in the conversations side menu

Now test with more than 5000 conversations. Use the new script to generate conversations. By default the script creates 100 conversations with kibana running at http://elastic:changeme@localhost:5601/kbn. Run it with --count 5001 to create a problematic amount of conversations. Optionally add a --kibana argument for a different user/password/url

node x-pack/plugins/elastic_assistant/scripts/create_conversations --count 5001 

Tire kicking:

  • Test creating new conversations from New chat, attack discovery, rules, alerts. Make sure your new conversations are shown at the top of the list.
  • Update and edit system prompts and their conversation assignments
  • Even with 5000+ conversations, you should no longer see the "Welcome conversation exists error" since we retrieve default conversations first.
  • Known bug. Now that you have more than 5000 conversations, check the conversations side menu. You will observe that OLD CONVO no longer shows. However, all the default conversations should still show at the bottom (or top if you update them)

@stephmilovic stephmilovic added release_note:fix v9.0.0 Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Team:Security Generative AI Security Generative AI v8.16.0 labels Oct 17, 2024
@stephmilovic stephmilovic marked this pull request as ready for review October 18, 2024 17:24
@stephmilovic stephmilovic requested a review from a team as a code owner October 18, 2024 17:24
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@stephmilovic stephmilovic added ci:cloud-deploy Create or update a Cloud deployment ci:project-deploy-security Create a Security Serverless Project labels Oct 18, 2024
@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

.option('kibana', {
type: 'string',
description: 'Kibana url including auth',
default: `http://elastic:changeme@localhost:5601/kbn`,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider removing the trailing /kbn

@stephmilovic
Copy link
Contributor Author

Closing in favor of #197305

@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 22, 2024

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

  • [job] [logs] Serverless Osquery Cypress Tests #3 / Alert Event Details "before each" hook for "should be able to run live query and add to timeline" "before each" hook for "should be able to run live query and add to timeline"

History

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:cloud-deploy Create or update a Cloud deployment ci:project-deploy-security Create a Security Serverless Project release_note:fix Team:Security Generative AI Security Generative AI Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants