-
Notifications
You must be signed in to change notification settings - Fork 30
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
Refactor for message action bar and add metrics for alerting summary #304
Changes from 45 commits
337dfd6
7938e5f
82c0fd4
19a5749
15f3d2a
578d8b8
876b68a
931ceaf
2e25985
21e2278
662923b
3ba3482
1956d49
14f9030
d247983
f799e15
d2278f1
f2516bb
d0f227d
a0e1a89
ae41d29
600f9f0
5f68dfa
07a5cff
5f27ef3
77ac476
4002c9b
15d44c3
14d6d0b
d94e244
cde988b
72aeb88
370b817
9eea26d
a8191e5
a35d147
53f7c4f
1d37a61
61da316
e7c2f7f
a56973a
9fafeae
2cd22d1
b665a42
b8bbda1
e99cfd9
c209f93
0bb77d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ import { | |
EuiFlexGroup, | ||
EuiFlexItem, | ||
EuiIcon, | ||
EuiIconTip, | ||
EuiLoadingContent, | ||
EuiMarkdownFormat, | ||
EuiPanel, | ||
|
@@ -20,21 +19,27 @@ import { | |
EuiTitle, | ||
} from '@elastic/eui'; | ||
import { useEffectOnce } from 'react-use'; | ||
import { MessageActions } from '../../tabs/chat/messages/message_action'; | ||
import { IncontextInsight as IncontextInsightInput } from '../../types'; | ||
import { getNotifications } from '../../services'; | ||
import { HttpSetup } from '../../../../../src/core/public'; | ||
import { SUMMARY_ASSISTANT_API } from '../../../common/constants/llm'; | ||
import shiny_sparkle from '../../assets/shiny_sparkle.svg'; | ||
import { UsageCollectionSetup } from '../../../../../src/plugins/usage_collection/public'; | ||
import { reportMetric } from '../../utils/report_metric'; | ||
|
||
export const GeneratePopoverBody: React.FC<{ | ||
incontextInsight: IncontextInsightInput; | ||
httpSetup?: HttpSetup; | ||
usageCollection?: UsageCollectionSetup; | ||
closePopover: () => void; | ||
}> = ({ incontextInsight, httpSetup, closePopover }) => { | ||
}> = ({ incontextInsight, httpSetup, usageCollection, closePopover }) => { | ||
const [summary, setSummary] = useState(''); | ||
const [insight, setInsight] = useState(''); | ||
const [insightAvailable, setInsightAvailable] = useState(false); | ||
const [showInsight, setShowInsight] = useState(false); | ||
const metricAppName = 'alertSumm'; | ||
|
||
const toasts = getNotifications().toasts; | ||
|
||
useEffectOnce(() => { | ||
|
@@ -98,6 +103,7 @@ export const GeneratePopoverBody: React.FC<{ | |
`Please provide your insight on this ${summaryType}.` | ||
); | ||
} | ||
reportMetric(usageCollection, metricAppName, 'generated'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we record if we want to record There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I'll change it to METRIC_TYPE.COUNT. |
||
}) | ||
.catch((error) => { | ||
toasts.addDanger( | ||
|
@@ -202,23 +208,34 @@ export const GeneratePopoverBody: React.FC<{ | |
const renderInnerFooter = () => { | ||
return ( | ||
<EuiPopoverFooter className="incontextInsightGeneratePopoverFooter" paddingSize="none"> | ||
<EuiFlexGroup gutterSize="none" direction={'rowReverse'}> | ||
{insightAvailable && ( | ||
<EuiFlexItem | ||
grow={false} | ||
onClick={() => { | ||
{ | ||
<div style={{ display: showInsight ? 'none' : 'block' }}> | ||
<MessageActions | ||
contentToCopy={summary} | ||
showFeedback | ||
showTraceIcon={insightAvailable} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems weird to pass insight related status to Trace Icon and its related action function. How about add a new parameter for the purpose of extending the MessageActions with customize buttons configuration? For this case, insight button should be passed in as a extending actions. You can think about which actions should be internal ones, and which should be extending ones. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like that the only difference between insight and trace is just the click function, so I think adding a new button is a little redundent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not exactly. insight button needs to show info "Insight With RAG" when moving your cursor over it. And what if you wants to have both trace button and insight button, or need other customized buttons like insight? Overall, they should be 2 different buttons. It could be an enhancement since this PR has been merged. |
||
onViewTrace={() => { | ||
setShowInsight(true); | ||
}} | ||
> | ||
<EuiIconTip | ||
aria-label="Insight" | ||
type="iInCircle" | ||
content="Insight with RAG" | ||
color={showInsight ? 'blue' : 'black'} | ||
/> | ||
</EuiFlexItem> | ||
)} | ||
</EuiFlexGroup> | ||
usageCollection={usageCollection} | ||
isOnTrace={showInsight} | ||
metricAppName={metricAppName} | ||
/> | ||
</div> | ||
} | ||
{ | ||
<div style={{ display: showInsight && insightAvailable ? 'block' : 'none' }}> | ||
<MessageActions | ||
contentToCopy={insight} | ||
showFeedback | ||
showTraceIcon={insightAvailable} | ||
onViewTrace={() => {}} | ||
usageCollection={usageCollection} | ||
isOnTrace={showInsight} | ||
metricAppName={metricAppName} | ||
/> | ||
</div> | ||
} | ||
</EuiPopoverFooter> | ||
Hailong-am marked this conversation as resolved.
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.
Does metric name have length restriction? If this is the case, you might need to add some protection check in util function.
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.
Actually I do not really know any specific restriction
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.
should we use full name
alertSummary
?