-
Notifications
You must be signed in to change notification settings - Fork 0
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
Store LLM output and update events timeline #59
Store LLM output and update events timeline #59
Conversation
benakansara
commented
Oct 28, 2024
- Stores LLM output into investigation Saved Object once analysis is completed
- If there is a stored LLM output on investigation SO, the events timeline will show significant events found by LLM
@@ -24,6 +24,7 @@ const updateInvestigationParamsSchema = z.object({ | |||
}), | |||
tags: z.array(z.string()), | |||
externalIncidentUrl: z.string().nullable(), | |||
automatedRcaAnalysis: z.array(z.any()).optional(), |
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.
let's make this an object: { events: z.array(z.any()) }
so we can easily add more top-level properties later
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.
updated
import { AnnotationEvent } from './annotation_event'; | ||
import { TIME_LINE_THEME } from './timeline_theme'; | ||
import { useFetchEvents } from '../../../../hooks/use_fetch_events'; | ||
import { useInvestigation } from '../../contexts/investigation_context'; | ||
import { useKibana } from '../../../../hooks/use_kibana'; | ||
import { AlertEvent } from './alert_event'; | ||
import { LLMEvent } from './llm_event'; |
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 should probably be more specific. maybe something like RootCauseAnalysisEventAnnotation
? also, if you want the root cause analysis event types, those types are exported from service_rca/index.ts
iirc.
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 have renamed LLMEvent
to RootCauseAnalysisEvent
, and event type from llm
to rca
. I am mapping RCA significant event here to existing one we are using for events timeline.
const [loading, setLoading] = useState(false); | ||
|
||
useEffect(() => { |
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 looks like I need to keep useEffect
that I previously reverted. If I update investigation SO in complete
function, it doesn't get updated properly every time.
'response' in event && 'report' in event.response && 'timeline' in event.response | ||
) | ||
) { | ||
await updateInvestigation({ |
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.
Sorry, I missed this, but we need to do it on the server, not on the client. Otherwise the user needs to keep the browser tab open.
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 didn't get why the user needs to keep browser tab open? It needs to be open only first time when storing the results to SO, afterwards it will just fetch events from SO unless you run the analysis again in which case it will rewrite results to SO.
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 that is the whole point, if the user runs it, they shouldn't be required to keep the browser tab open.
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 reason for storing LLM result was that when you visit the page again, you don't have to run the analysis and wait for few minutes. But when you run the analysis for the first time, you would want to see the result, isn't it? So imo it's ok to keep the browser tab open whenever new analysis is being run. Another aspect is that if user closes the tab, that would mean they don't want to continue with the analysis, right? We should not carry on with the requests to LLM in that case.
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.
No, as I said before, the point is that the user needs to be able to navigate away from the page and the results will be ready when they get back. Why ask them to keep a browser tab open for minutes and make sure they stay on the same actual page?