Skip to content
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

Merged
merged 48 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from 44 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
337dfd6
Add message action module
000FLMS Aug 7, 2024
7938e5f
Finish basic function of feedback buttons
000FLMS Aug 9, 2024
82c0fd4
Add test cases and fix bugs
000FLMS Aug 12, 2024
19a5749
Refactor the code to deal with core loss and fix the regenerate bug
000FLMS Aug 14, 2024
15f3d2a
Update CHANGELOG.md
000FLMS Aug 19, 2024
578d8b8
Update for smaller buttons
000FLMS Aug 19, 2024
876b68a
Merge branch 'main' into main
000FLMS Aug 19, 2024
931ceaf
Update for ui-metric
000FLMS Aug 30, 2024
2e25985
Support insight with RAG
qianheng-aws Aug 28, 2024
21e2278
Add change in CHANGELOG.md
qianheng-aws Aug 30, 2024
662923b
Put footer in the same panel with content to match UX design
qianheng-aws Aug 30, 2024
3ba3482
Refine alert prompt
qianheng-aws Sep 2, 2024
1956d49
set CSS scrollbar-width to thin
qianheng-aws Sep 2, 2024
14f9030
Hide insight agent id from front-end
qianheng-aws Sep 2, 2024
d247983
Merge branch 'insightWithRAG' into alertingInsight
000FLMS Sep 2, 2024
f799e15
Change summary agent config id
qianheng-aws Sep 3, 2024
d2278f1
Address comments
qianheng-aws Sep 4, 2024
f2516bb
Incorporate message action buttons into alerting summary
000FLMS Sep 4, 2024
d0f227d
Add hover tips and rearrange buttons
000FLMS Sep 4, 2024
a0e1a89
Fix UT
qianheng-aws Sep 4, 2024
ae41d29
Merge branch 'main' into main
000FLMS Sep 5, 2024
600f9f0
Add ui-metric for alerting summary
000FLMS Sep 5, 2024
5f68dfa
fix testing bugs
000FLMS Sep 5, 2024
07a5cff
Merge remote-tracking branch 'refs/remotes/origin/main' into insightW…
qianheng-aws Sep 5, 2024
5f27ef3
Merge branch 'main' into main
000FLMS Sep 5, 2024
77ac476
Merge branch 'main' into main
000FLMS Sep 9, 2024
4002c9b
Merge remote-tracking branch 'refs/remotes/origin/main' into insightW…
qianheng-aws Sep 9, 2024
15d44c3
Change agent execute API
qianheng-aws Sep 9, 2024
14d6d0b
Remove prompt from node JS server
qianheng-aws Sep 9, 2024
d94e244
Merge branch 'main' into insightWithRAG
qianheng-aws Sep 10, 2024
cde988b
Replace CSS with component property
qianheng-aws Sep 10, 2024
72aeb88
Save the status when switch between summary and insight
000FLMS Sep 11, 2024
370b817
Merge branch 'insightWithRAG' of github.com:qianheng-aws/dashboards-a…
000FLMS Sep 11, 2024
9eea26d
Merge branch 'opensearch-project:main' into main
000FLMS Sep 11, 2024
a8191e5
Merge branch 'main' of https://github.com/000FLMS/dashboards-assistan…
000FLMS Sep 11, 2024
a35d147
Add tests for metrics
000FLMS Sep 12, 2024
53f7c4f
Merge branch 'main' into alertingInsight
000FLMS Sep 12, 2024
1d37a61
Merge branch 'main' into alertingInsight
000FLMS Sep 13, 2024
61da316
Merge branch 'main' into alertingInsight
000FLMS Sep 13, 2024
e7c2f7f
Use usageCollectionPluginMock in tests
000FLMS Sep 18, 2024
a56973a
Merge branch 'alertingInsight' of https://github.com/000FLMS/dashboar…
000FLMS Sep 18, 2024
9fafeae
Merge branch 'main' into alertingInsight
000FLMS Sep 19, 2024
2cd22d1
Refactor reportMetric as util funciton
000FLMS Sep 19, 2024
b665a42
Merge branch 'alertingInsight' of https://github.com/000FLMS/dashboar…
000FLMS Sep 19, 2024
b8bbda1
Remove telementry plugin
000FLMS Sep 19, 2024
e99cfd9
Merge branch 'main' into alertingInsight
000FLMS Sep 20, 2024
c209f93
Fix metric type
000FLMS Sep 20, 2024
0bb77d8
Merge branch 'alertingInsight' of https://github.com/000FLMS/dashboar…
000FLMS Sep 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,5 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Use smaller and compressed variants of buttons and form components ([#250](https://github.com/opensearch-project/dashboards-assistant/pull/250))
- Support insight with RAG in alert analysis assistant and refine the UX ([#266](https://github.com/opensearch-project/dashboards-assistant/pull/266))
- Add assistant enabled capabilities to control rendering component([#267](https://github.com/opensearch-project/dashboards-assistant/pull/267))
- Add data to summary API([#295](https://github.com/opensearch-project/dashboards-assistant/pull/295))
- Add data to summary API([#295](https://github.com/opensearch-project/dashboards-assistant/pull/295))
- Refactor popover to add message action bar and add metrics to thumb-up and thumb-down([#304](https://github.com/opensearch-project/dashboards-assistant/pull/304))
4 changes: 3 additions & 1 deletion opensearch_dashboards.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
],
"optionalPlugins": [
"dataSource",
"dataSourceManagement"
"dataSourceManagement",
"usageCollection",
"telemetry"
Hailong-am marked this conversation as resolved.
Show resolved Hide resolved
],
"requiredBundles": [],
"configPath": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
*/

import React from 'react';
import { render, cleanup, fireEvent, waitFor } from '@testing-library/react';
import { render, cleanup, fireEvent, waitFor, screen } from '@testing-library/react';
import { getConfigSchema, getNotifications } from '../../services';
import { GeneratePopoverBody } from './generate_popover_body';
import { HttpSetup } from '../../../../../src/core/public';
import { SUMMARY_ASSISTANT_API } from '../../../common/constants/llm';
import { usageCollectionPluginMock } from '../../../../../src/plugins/usage_collection/public/mocks';

jest.mock('../../services');

Expand All @@ -26,7 +27,7 @@ beforeEach(() => {
});

afterEach(cleanup);

const mockUsageCollection = usageCollectionPluginMock.createSetupContract();
const mockPost = jest.fn();
const mockHttpSetup: HttpSetup = ({
post: mockPost,
Expand Down Expand Up @@ -68,6 +69,7 @@ describe('GeneratePopoverBody', () => {
incontextInsight={incontextInsightMock}
httpSetup={mockHttpSetup}
closePopover={closePopoverMock}
usageCollection={mockUsageCollection}
/>
);

Expand All @@ -86,9 +88,15 @@ describe('GeneratePopoverBody', () => {
expect(queryByLabelText('loading_content')).toBeNull();
expect(mockPost).toHaveBeenCalledWith(SUMMARY_ASSISTANT_API.SUMMARIZE, expect.any(Object));
expect(mockToasts.addDanger).not.toHaveBeenCalled();
// generated metric is sent
expect(mockUsageCollection.reportUiStats).toHaveBeenCalledWith(
'alertSumm',
'click',
expect.stringMatching(/^generated/)
);

// insight tip icon is visible
const insightTipIcon = getByLabelText('Insight');
const insightTipIcon = screen.getAllByLabelText('How was this generated?')[0];
expect(insightTipIcon).toBeInTheDocument();

// 2. Click insight tip icon to view insight
Expand Down Expand Up @@ -142,6 +150,7 @@ describe('GeneratePopoverBody', () => {
incontextInsight={incontextInsightMock}
httpSetup={mockHttpSetup}
closePopover={closePopoverMock}
usageCollection={mockUsageCollection}
/>
);

Expand All @@ -159,9 +168,15 @@ describe('GeneratePopoverBody', () => {
expect(queryByLabelText('loading_content')).toBeNull();
expect(mockPost).toHaveBeenCalledWith(SUMMARY_ASSISTANT_API.SUMMARIZE, expect.any(Object));
expect(mockToasts.addDanger).not.toHaveBeenCalled();
// generated metric is sent
expect(mockUsageCollection.reportUiStats).toHaveBeenCalledWith(
'alertSumm',
'click',
expect.stringMatching(/^generated/)
);

// insight tip icon is not visible
expect(queryByLabelText('Insight')).toBeNull();
expect(screen.queryAllByLabelText('How was this generated?')).toHaveLength(0);
// Only call http post 1 time.
expect(mockPost).toHaveBeenCalledTimes(1);
});
Expand Down Expand Up @@ -223,6 +238,6 @@ describe('GeneratePopoverBody', () => {
// Show summary content although insight generation failed
expect(getByText('Generated summary content')).toBeInTheDocument();
// insight tip icon is not visible for this alert
expect(queryByLabelText('Insight')).toBeNull();
expect(screen.queryAllByLabelText('How was this generated?')).toHaveLength(0);
});
});
51 changes: 34 additions & 17 deletions public/components/incontext_insight/generate_popover_body.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
EuiFlexGroup,
EuiFlexItem,
EuiIcon,
EuiIconTip,
EuiLoadingContent,
EuiMarkdownFormat,
EuiPanel,
Expand All @@ -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';
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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?


const toasts = getNotifications().toasts;

useEffectOnce(() => {
Expand Down Expand Up @@ -98,6 +103,7 @@ export const GeneratePopoverBody: React.FC<{
`Please provide your insight on this ${summaryType}.`
);
}
reportMetric(usageCollection, metricAppName, 'generated');
Copy link
Collaborator

@Hailong-am Hailong-am Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we record generated metric, i think the metric type could be METRIC_TYPE.COUNT.

if we want to record click metric, it should not care the result of agent execution.

Copy link
Contributor Author

@000FLMS 000FLMS Sep 20, 2024

Choose a reason for hiding this comment

The 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(
Expand Down Expand Up @@ -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}
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@qianheng-aws qianheng-aws Sep 20, 2024

Choose a reason for hiding this comment

The 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
);
};
Expand Down
9 changes: 8 additions & 1 deletion public/components/incontext_insight/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,20 @@ import chatIcon from '../../assets/chat.svg';
import sparkle from '../../assets/sparkle.svg';
import { HttpSetup } from '../../../../../src/core/public';
import { GeneratePopoverBody } from './generate_popover_body';
import { UsageCollectionSetup } from '../../../../../src/plugins/usage_collection/public/plugin';

export interface IncontextInsightProps {
children?: React.ReactNode;
httpSetup?: HttpSetup;
usageCollection?: UsageCollectionSetup;
}

// TODO: add saved objects / config to store seed suggestions
export const IncontextInsight = ({ children, httpSetup }: IncontextInsightProps) => {
export const IncontextInsight = ({
children,
httpSetup,
usageCollection,
}: IncontextInsightProps) => {
const anchor = useRef<HTMLDivElement>(null);
const [isVisible, setIsVisible] = useState(false);

Expand Down Expand Up @@ -279,6 +285,7 @@ export const IncontextInsight = ({ children, httpSetup }: IncontextInsightProps)
<GeneratePopoverBody
incontextInsight={input}
httpSetup={httpSetup}
usageCollection={usageCollection}
closePopover={closePopover}
/>
);
Expand Down
Loading