-
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
[Obs AI Assistant] Implement contextual actions #178405
[Obs AI Assistant] Implement contextual actions #178405
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
/ci |
Pinging @elastic/obs-knowledge-team (Team:obs-knowledge) |
…ar/kibana into obs-ai-assistant-screen-actions
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.
Cool stuff!
@@ -4,6 +4,8 @@ | |||
* 2.0; you may not use this file except in compliance with the Elastic License | |||
* 2.0. | |||
*/ | |||
import type { ObservabilityAIAssistantChatService } from '../public'; |
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.
Is it allowed for common
to import from public?
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.
for types, yes
@@ -77,11 +79,37 @@ export interface KnowledgeBaseEntry { | |||
role: KnowledgeBaseEntryRole; | |||
} | |||
|
|||
export interface ObservabilityAIAssistantScreenContextRequest { |
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.
Off topic: Is it needed to prefix so many types with ObservabilityAIAssistant
?
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.
auto-imports search across all projects so in the end I think it's better to be more verbose... e.g. try to import KibanaReactProvider
and see what comes up 😄
actions?: Array<{ name: string; description: string; parameters?: CompatibleJSONSchema }>; | ||
} | ||
|
||
export type ScreenContextActionRespondFunction<TArguments extends unknown> = ({}: { |
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.
Nit: Would it be wrong to just call this ScreenContextAction
? or ScreenContextFunction
?
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.
it's an action and then it's the respond property of the action. I do understand it's verbose and I'm not too happy with it but I think it's better than those alternatives
name: string; | ||
description: string; | ||
parameters?: CompatibleJSONSchema; | ||
respond: ScreenContextActionRespondFunction<TArguments>; |
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.
Off topic: It's interesting to see how the language gets mixed between it being an "action" (function) and a "part of conversation" (respond).
}); | ||
|
||
it('the observable errors out', async () => { | ||
await expect(async () => await lastValueFrom(callComplete())).rejects.toThrowError( |
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.
Is it needed to make the call 3 times to check each thing?
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.
mostly an artifact of expect
and rejects
. open to alternatives but couldn't easily figure out a better way.
if (functionClient.hasAction(functionCallName)) { | ||
this.dependencies.logger.debug(`Executing client-side action: ${functionCallName}`); | ||
|
||
functionClient.validate( |
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.
This throws an error if it fails? And if it fails, then what happens with the subscriber
? Does it not complete?
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.
the error falls through and eventually subscriber.error is called. but that shouldn't actually happen here, we should inject a function error message and send it back to the LLM. will fix and add a test, thanks.
const originalPushState = window.history.pushState.bind(window.history); | ||
const originalReplaceState = window.history.replaceState.bind(window.history); |
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.
Do we need to revert these as part of a clean up for this hook?
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.
it's hard to clean up because something else might have overridden window.history.pushState
at that point. I'll add a flag for the setState
calls and clean up the window listeners.
window.addEventListener('popstate', () => { | ||
setHref(window.location.href); | ||
}); | ||
|
||
window.addEventListener('hashchange', () => { | ||
setHref(window.location.href); | ||
}); |
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.
And clean these up maybe as well?
createScreenContextAction( | ||
{ | ||
name: 'foo', | ||
description: 'foo', | ||
parameters: { | ||
type: 'object', | ||
properties: { | ||
foo: { | ||
type: 'string', | ||
}, | ||
}, | ||
}, | ||
}, | ||
async ({}) => { | ||
return { | ||
content: 'Action succesfully executed', | ||
}; | ||
} | ||
), |
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 guess this is to clean up?
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.
yes (thanks :) )
|
||
const conversations = await observabilityAIAssistantAPIClient | ||
.writeUser({ | ||
endpoint: 'POST /internal/observability_ai_assistant/conversations', |
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.
Why is this POST rather than GET?
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.
it's a search request, with optionally a body with filters (right now just a query string).
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
#178405 made some changes to how the chat service passes data to the API, while doing this we missed to pass along the `responseLanguage` setting, breaking the language selection feature. This PR just wires those up again and adds a test to catch this for the future.
Implements contextual actions. With this change, consumers of the Observability AI Assistant can add contextual actions to screen context, which are functions that are executed on the client. Its results are then pushed back to the API and the LLM to continue the conversation. This allows consumers to do things like:
Additionally, this also opens up the possibility for consumers of the complete API to implement their own functions.