-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[AI Assistant] Add setting for preferred type #179233
[AI Assistant] Add setting for preferred type #179233
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
/ci |
b179bdc
to
13a024d
Compare
/ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
One question: When
Pinging @elastic/obs-knowledge-team (Team:obs-knowledge) |
@kc13greiner want to finish that sentence? 😀 |
😆 I figured it out; I had the wrong proj type running - PR LGTM! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review only, LGTM. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Solution team didn't get a feedback from the Platform team regarding AI Assistant global availability in Kibana and didn't make a decision for making it deliverable in 8.14.
Also we want to avoid a UX, where selecting Security as preferred AI Assistant will do nothing to the user (CC: @jamesspi)
Screen.Recording.2024-03-25.at.4.00.47.PM.mov
@YulNaumenko as mentioned on Slack, we did get confirmation. Re:
As mentioned in the description, each solution needs to implement something that respects the setting. For Observability, that happens in this PR. For Security, that should probably happen in a follow-up. I can also remove Security as an option for now. What do you prefer? In any case, I want to avoid blocking this PR until you have implemented something on your side, as we have follow-up PRs planned. |
@dgieselaar @boriskirov Would you mind sharing any Figma files/explorations for context? I also have a few other questions:
|
@bojanasan all good points and appreciate the feedback. Re: the setting, let's first figure out whether we'll want to use that approach short-term. I did ask in the Design WG channel about whether we can come up with a way to display it in the top nav without losing discoverability. I don't think we have other options than the top nav: anything outside of the top nav gets unmounted and remounted so you will lose the state of the conversation as you move between apps. This is especially an issue in docked mode. |
@dgieselaar Thanks for the quick response 👍🏻 I had noticed that note in slack although it lacked the context that I needed, so it took a bit of time for me to understand the implication of the proposal.
happy to take a look at any rough notes/sketches if that's faster for catching up on the context |
@bojanasan re:
This is about having an advanced setting at all (to select the preferred Assistant type). So not necessarily about whether the setting is displayed - I think we can tackle that later.
Yes. The top nav is always there - the header below that is rendered by each app separately. That means if you move between apps, the AI Assistant component is unmounted, which means that all state is lost - things that are kept in memory, network requests that are running etc. Effectively, it means the same as refreshing the page. If a conversation has not been stored, if the LLM is currently still responding, or a function is being executed, that is then aborted and the conversation is reset to its last persisted state after remounting the component. |
@YulNaumenko as discussed, I've labeled it as an Observability-only setting. The setting name is still generic - that way we can simply add Security to the list and remove the Observability-specific labeling once you folks are ready for it (otherwise we'd have to add a new setting and remove this one). @bojanasan for reference, we agreed we'd ship this as an Observability-only setting for now and don't advertise it. We can talk about how can polish this when Security is ready, this is a very minimal implementation for now given the changes we expect. |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @dgieselaar |
LGTM! Thank you, @dgieselaar for making the changes. |
Adds a setting in the
aiAssistantManagementSelection
plugin that allows users to set the preferred Assistant type. The allowed values are:default
: the status quo, which is: show the Observability AI Assistant in Observability apps, the Security AI Assistant in Security solution apps, and none in the other apps.never
: Never show any AI Assistant.observability
: Show the Observability AI Assistant everywhere, except for Security solution apps.security
:Show the Security AI Assistant everywhere, except for Observability apps.it is up to the solutions to respect the setting. See x-pack/plugins/observability_solution/observability_ai_assistant_app/public/hooks/is_nav_control_visible.tsx for an example. It should also be set in the specific Serverless config: config/serverless.oblt.yml.
Update: this is now labeled as an Observability-only setting.