-
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
[Observability AI Assistant] Create first Starter prompts #178907
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/obs-knowledge-team (Team:obs-knowledge) |
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
2d71c50
to
7567cf7
Compare
x-pack/plugins/observability_solution/observability/public/application/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/slo/public/pages/slos/components/slo_list.tsx
Outdated
Show resolved
Hide resolved
...s/observability_solution/observability_ai_assistant_app/public/components/chat/chat_body.tsx
Outdated
Show resolved
Hide resolved
...rvability_solution/observability_ai_assistant_app/public/components/chat/starter_prompts.tsx
Outdated
Show resolved
Hide resolved
...rvability_solution/observability_ai_assistant_app/public/components/chat/welcome_message.tsx
Outdated
Show resolved
Hide resolved
...k/plugins/observability_solution/observability_ai_assistant_app/public/utils/non_nullable.ts
Show resolved
Hide resolved
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!
d288c27
to
7643392
Compare
x-pack/plugins/observability_solution/observability/public/application/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/observability_ai_assistant/public/plugin.tsx
Outdated
Show resolved
Hide resolved
...rvability_solution/observability_ai_assistant_app/public/components/chat/starter_prompts.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/slo/public/application.tsx
Outdated
Show resolved
Hide resolved
|
||
const { | ||
ruleTypesState: { data: ruleTypes }, | ||
} = useLoadRuleTypesQuery({ |
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.
useLoadRuleTypesQuery()
returns id
s and a name
, but no description
}); | ||
|
||
return useMemo(() => { | ||
const ruleTypesFromRuleTypeRegistry = ruleTypeRegistry.list(); |
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.
ruleTypeRegistry.list()
returns id
s and description
s, but no name
😅
x-pack/plugins/observability_solution/observability/public/pages/alerts/alerts.tsx
Outdated
Show resolved
Hide resolved
6f9eeb6
to
819ff0c
Compare
.../plugins/observability_solution/observability_ai_assistant/public/content/starter_prompts.ts
Outdated
Show resolved
Hide resolved
|
||
const starterPrompts = uniq( | ||
[...contexts] | ||
.reverse() |
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.
I don't think you want to order this. We cannot depend on order, because components get mounted and unmounted, changing the order of when things are registered
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.
Interesting. While working and testing the feature I haven't seen it not working. Can you give an example of when this does not work?
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.
What do you expect the order to be based on? You can probably fairly easily reproduce this by adding useEffect hooks that register starting prompts, and invalidating the dependencies for one on every render, and noticing that that will always come up on top.
@@ -118,21 +119,52 @@ export function ApmMainTemplate({ | |||
isServerless: config?.serverlessOnboarding, | |||
}); | |||
|
|||
let screenDescription = ''; |
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.
Could we move the AI assistant code to its own file?
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.
Same comments for the other files. I think we could clean up the components.
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.
Not really. This functionality is intended for other plugins to give contextual information to the assistant about what the user is looking at right now.
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.
I think what @cauemarcondes means is that it could be a separate hook or something similar. I've also done that in other places in APM (e.g. getThroughputScreenContext
).
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.
I saw those and didn't think it was super readable. But if its important for you, I will oblige
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.
IMO it keeps the component clean and small making it easy to maintain.
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.
IMO jumping between files hinders readability and leads engineers to just leave it alone because they don't see the contents anymore. Many, many examples of this in the Kibana codebase abound.
As stated though I will update.
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.
IMO jumping between files hinders readability and leads engineers to just leave it alone because they don't see the contents anymore
I tend to agree and disagree with that 😄. The screen context/description is very specific for the AI assistant to know about this page/component. Me as a developer who is actively working on these components/pages I don't care what the assistant is doing. I know they are there though. What I want is to see a clean component, not having a bunch of static text on my way 😆.
For you, I think it'll be even easier as you'll have a clean file with only AI stuff.
...plugins/observability_solution/apm/public/components/routing/templates/apm_main_template.tsx
Outdated
Show resolved
Hide resolved
3316cbf
to
37e89df
Compare
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.
infra
changes LGTM
return setScreenContext?.({ | ||
screenDescription: hasNoLocations | ||
? 'The user has no locations configured.' | ||
: `The user has ${locations.length} locations configured: ${JSON.stringify(locations)}`, |
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.
any idea how much tokens this is?
return setScreenContext?.({ | ||
data: ruleTypesWithDescriptions.map((rule) => ({ | ||
name: rule.id, | ||
value: `${rule.name} ${rule.description}`, |
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.
you can use value: rule
. In this case it probably is overkill to use data
(I wasn't aware of the shape, sorry). But it's also fine to leave it as-is, there's not a lot of tokens so it gets sent over automatically.
@elasticmachine merge upstream |
I've created a follow-up issue for the ordering issue: #180698. |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @CoenWarmer |
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.
Obs UX LGTM
Summary
This introduces starter prompts for users to get the conversation going.
Allows Kibana app developers to create starter prompts at any time using the
setScreenContext
method.Up to 4 starter prompts will be shown to the user. Starter Prompts added by an app take precedence over the default starter prompts.
Screen.Recording.2024-04-05.at.15.15.48.mov
Default Starter Prompts (visible in all Observability apps when no additional prompts have been registered by app / page)
Give me examples of questions I can ask here.
Can you explain this page?
Do I have any alerts?
What are SLOs?
Starter Prompts added to different specific Observability apps
HasDataProvider
returns no data for an appWhy don't I see any data for the {appsWithoutData} sections?
Can you explain the rule types that are available?
Can you explain the rule types that are available?
Why don't I see any data?
Why don't I see any data?
Why don't I see any data?
Why don't I see any monitors?
Why don't I see any data?
Guidance for Kibana engineers
The team owning a plugin can add the following code to add a starter prompt to the Assistant:
You can use the optional
screenDescription
anddata
keys insetScreenContext
to pass along additional information to the LLM which may be beneficial in answering the starter prompt that you configure. For example:As a rule of thumb, the more generic or 'high level' the starter prompt is, the higher in the React app tree it should be added.
More specific starter prompts that are relevant for pages (or even sections inside pages) should be added by adding
setScreenContext
further down in the React app tree, for instance in page components or even more specific. Be aware that only 4 starter prompts will be displayed so if you place more than that across components then they will not be displayed.For instance, in the case of the Observability app, the guidance would be:
What is Observability?
renderApp
What are SLOs?
renderApp
How do I set up Alerts?
renderApp
Can you describe the different rule types?
Can you help me configure an SLO?
Do I have any misconfigured SLOs?
Please validate whether or not the response given by the LLM is correct and helpful for the user.
If it does not, please contact the Observability AI Assistant team as there are multiple techniques to make the LLMs answer smarter. We can work with you to find the way that is most effective.