-
Notifications
You must be signed in to change notification settings - Fork 23
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
T2viz UI metrics #312
T2viz UI metrics #312
Changes from all commits
cde949f
4559e63
4f107e9
25442d1
9478056
c56f0d3
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 |
---|---|---|
@@ -0,0 +1,79 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import React from 'react'; | ||
import { render, screen, fireEvent, waitFor } from '@testing-library/react'; | ||
import { METRIC_TYPE } from '@osd/analytics'; | ||
|
||
import { FeedbackThumbs } from './feedback_thumbs'; | ||
|
||
describe('<FeedbackThumbs />', () => { | ||
it('should report thumbs up metric', () => { | ||
const usageCollectionMock = { | ||
reportUiStats: jest.fn(), | ||
METRIC_TYPE, | ||
}; | ||
|
||
render(<FeedbackThumbs usageCollection={usageCollectionMock} appName="test-app" />); | ||
fireEvent.click(screen.getByLabelText('ThumbsUp')); | ||
expect(usageCollectionMock.reportUiStats).toHaveBeenCalledWith( | ||
'test-app', | ||
METRIC_TYPE.CLICK, | ||
expect.stringMatching(/thumbs_up.*/) | ||
); | ||
}); | ||
|
||
it('should report thumbs down metric', () => { | ||
const usageCollectionMock = { | ||
reportUiStats: jest.fn(), | ||
METRIC_TYPE, | ||
}; | ||
|
||
render(<FeedbackThumbs usageCollection={usageCollectionMock} appName="test-app" />); | ||
fireEvent.click(screen.getByLabelText('ThumbsDown')); | ||
expect(usageCollectionMock.reportUiStats).toHaveBeenCalledWith( | ||
'test-app', | ||
METRIC_TYPE.CLICK, | ||
expect.stringMatching(/thumbs_down.*/) | ||
); | ||
}); | ||
|
||
it('should only report metric only once', () => { | ||
const usageCollectionMock = { | ||
reportUiStats: jest.fn(), | ||
METRIC_TYPE, | ||
}; | ||
|
||
render(<FeedbackThumbs usageCollection={usageCollectionMock} appName="test-app" />); | ||
// click the button two times | ||
fireEvent.click(screen.getByLabelText('ThumbsDown')); | ||
fireEvent.click(screen.getByLabelText('ThumbsDown')); | ||
expect(usageCollectionMock.reportUiStats).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
it('should hide thumbs down button after thumbs up been clicked', () => { | ||
const usageCollectionMock = { | ||
reportUiStats: jest.fn(), | ||
METRIC_TYPE, | ||
}; | ||
|
||
render(<FeedbackThumbs usageCollection={usageCollectionMock} appName="test-app" />); | ||
|
||
fireEvent.click(screen.getByLabelText('ThumbsUp')); | ||
expect(screen.queryByLabelText('ThumbsDown')).toBeNull(); | ||
}); | ||
|
||
it('should hide thumbs up button after thumbs down been clicked', () => { | ||
const usageCollectionMock = { | ||
reportUiStats: jest.fn(), | ||
METRIC_TYPE, | ||
}; | ||
|
||
render(<FeedbackThumbs usageCollection={usageCollectionMock} appName="test-app" />); | ||
|
||
fireEvent.click(screen.getByLabelText('ThumbsDown')); | ||
expect(screen.queryByLabelText('ThumbsUp')).toBeNull(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { EuiButtonIcon, EuiFlexGroup, EuiFlexItem } from '@elastic/eui'; | ||
import React, { useState } from 'react'; | ||
import { v4 as uuidv4 } from 'uuid'; | ||
|
||
import { UsageCollectionStart } from '../../../../src/plugins/usage_collection/public'; | ||
|
||
interface Props { | ||
appName: string; | ||
usageCollection: UsageCollectionStart; | ||
className?: string; | ||
} | ||
|
||
export const FeedbackThumbs = ({ usageCollection, appName, className }: Props) => { | ||
const [feedback, setFeedback] = useState<'thumbs_up' | 'thumbs_down' | undefined>(); | ||
|
||
const onFeedback = (eventName: 'thumbs_up' | 'thumbs_down') => { | ||
// Only send metric if no current feedback set | ||
if (!feedback) { | ||
usageCollection.reportUiStats( | ||
appName, | ||
usageCollection.METRIC_TYPE.CLICK, | ||
`${eventName}-${uuidv4()}` | ||
); | ||
setFeedback(eventName); | ||
} | ||
}; | ||
|
||
return ( | ||
<EuiFlexGroup gutterSize="none" className={className}> | ||
{(!feedback || feedback === 'thumbs_up') && ( | ||
<EuiFlexItem> | ||
<EuiButtonIcon | ||
size="xs" | ||
color={feedback === 'thumbs_up' ? 'primary' : 'text'} | ||
iconType="thumbsUp" | ||
aria-label="ThumbsUp" | ||
onClick={() => onFeedback('thumbs_up')} | ||
/> | ||
</EuiFlexItem> | ||
)} | ||
{(!feedback || feedback === 'thumbs_down') && ( | ||
<EuiFlexItem> | ||
<EuiButtonIcon | ||
size="xs" | ||
color={feedback === 'thumbs_down' ? 'primary' : 'text'} | ||
iconType="thumbsDown" | ||
aria-label="ThumbsDown" | ||
onClick={() => onFeedback('thumbs_down')} | ||
/> | ||
</EuiFlexItem> | ||
)} | ||
</EuiFlexGroup> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,8 +38,8 @@ export class Text2Vega { | |
this.result$ = this.input$ | ||
.pipe( | ||
filter((v) => v.prompt.length > 0), | ||
debounceTime(200), | ||
tap(() => this.status$.next('RUNNING')) | ||
tap(() => this.status$.next('RUNNING')), | ||
debounceTime(200) | ||
Comment on lines
+41
to
+42
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. any reason for changing the order? 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. Sometimes, it causes UI flashing. |
||
) | ||
.pipe( | ||
switchMap((v) => | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,4 +16,11 @@ | |
padding-top: 15px; | ||
padding-left: 30px; | ||
} | ||
|
||
.feedback_thumbs { | ||
position: absolute; | ||
right: 16px; | ||
top: 4px; | ||
Comment on lines
+22
to
+23
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. nit: $euiSize is 16px |
||
z-index: 9999; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ import { | |
} from '@elastic/eui'; | ||
import React, { useEffect, useMemo, useRef, useState } from 'react'; | ||
import { i18n } from '@osd/i18n'; | ||
import { v4 as uuidv4 } from 'uuid'; | ||
|
||
import { useCallback } from 'react'; | ||
import { useObservable } from 'react-use'; | ||
|
@@ -46,9 +47,10 @@ import { getIndexPatterns } from '../../services'; | |
import { NLQ_VISUALIZATION_EMBEDDABLE_TYPE } from './embeddable/nlq_vis_embeddable'; | ||
import { NLQVisualizationInput } from './embeddable/types'; | ||
import { EditorPanel } from './editor_panel'; | ||
import { VIS_NLQ_SAVED_OBJECT } from '../../../common/constants/vis_type_nlq'; | ||
import { VIS_NLQ_APP_ID, VIS_NLQ_SAVED_OBJECT } from '../../../common/constants/vis_type_nlq'; | ||
import { HeaderVariant } from '../../../../../src/core/public'; | ||
import { TEXT2VEGA_INPUT_SIZE_LIMIT } from '../../../common/constants/llm'; | ||
import { FeedbackThumbs } from '../feedback_thumbs'; | ||
|
||
export const Text2Viz = () => { | ||
const { savedObjectId } = useParams<{ savedObjectId?: string }>(); | ||
|
@@ -68,9 +70,23 @@ export const Text2Viz = () => { | |
uiSettings, | ||
savedObjects, | ||
config, | ||
usageCollection, | ||
}, | ||
} = useOpenSearchDashboards<StartServices>(); | ||
|
||
/** | ||
* Report metrics when the application is loaded | ||
*/ | ||
useEffect(() => { | ||
if (usageCollection) { | ||
usageCollection.reportUiStats( | ||
VIS_NLQ_APP_ID, | ||
usageCollection.METRIC_TYPE.LOADED, | ||
`app_loaded-${uuidv4()}` | ||
); | ||
} | ||
}, [usageCollection]); | ||
|
||
const useUpdatedUX = uiSettings.get('home:useNewHomePage'); | ||
|
||
const [input, setInput] = useState(''); | ||
|
@@ -112,14 +128,23 @@ export const Text2Viz = () => { | |
}); | ||
} else { | ||
setEditorInput(JSON.stringify(result, undefined, 4)); | ||
|
||
// Report metric when visualization generated successfully | ||
if (usageCollection) { | ||
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. Could use 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. Currently, it doesn't record the failures. If we do need to record the failures, I'd rather use another event type, like: usageCollection.reportUiStats(
VIS_NLQ_APP_ID,
usageCollection.METRIC_TYPE.LOADED,
`generate_failed-${uuidv4()}`
); What do you think? 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. I mean we could use the same metric, and emit 0 when fails, 1 when succeeds. Like
We always ensure that we emit one time for a fetch, it's either 1 or 0. |
||
usageCollection.reportUiStats( | ||
VIS_NLQ_APP_ID, | ||
usageCollection.METRIC_TYPE.LOADED, | ||
`generated-${uuidv4()}` | ||
); | ||
} | ||
} | ||
} | ||
}); | ||
|
||
return () => { | ||
subscription.unsubscribe(); | ||
}; | ||
}, [http, notifications]); | ||
}, [http, notifications, usageCollection]); | ||
|
||
/** | ||
* Loads the saved object from id when editing an existing visualization | ||
|
@@ -243,6 +268,15 @@ export const Text2Viz = () => { | |
}), | ||
}); | ||
dialog.close(); | ||
|
||
// Report metric when a new visualization is saved. | ||
if (usageCollection) { | ||
usageCollection.reportUiStats( | ||
VIS_NLQ_APP_ID, | ||
usageCollection.METRIC_TYPE.LOADED, | ||
`saved-${uuidv4()}` | ||
); | ||
Comment on lines
+273
to
+278
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. out of curiosity, how we are going to use those metrics? For example, how to get the statics of how many user save the visualization? the event name is with a uuid, sounds like not suitable for aggregation. 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. I guess we don't need it any more? @chishui any idea? 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.
You mean uuid or metric itself? We need both |
||
} | ||
} | ||
} catch (e) { | ||
notifications.toasts.addDanger({ | ||
|
@@ -270,7 +304,7 @@ export const Text2Viz = () => { | |
/> | ||
) | ||
); | ||
}, [notifications, vegaSpec, input, overlays, selectedSource, savedObjectId]); | ||
}, [notifications, vegaSpec, input, overlays, selectedSource, savedObjectId, usageCollection]); | ||
|
||
const pageTitle = savedObjectId | ||
? i18n.translate('dashboardAssistant.feature.text2viz.breadcrumbs.editVisualization', { | ||
|
@@ -412,6 +446,13 @@ export const Text2Viz = () => { | |
paddingSize="none" | ||
scrollable={false} | ||
> | ||
{usageCollection ? ( | ||
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. IIUC, usageCollection is always available, the feature flag only controls whether it can send metrics to server. 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.
|
||
<FeedbackThumbs | ||
usageCollection={usageCollection} | ||
appName={VIS_NLQ_APP_ID} | ||
className="feedback_thumbs" | ||
/> | ||
) : null} | ||
<EmbeddableRenderer factory={factory} input={visInput} /> | ||
</EuiResizablePanel> | ||
<EuiResizableButton /> | ||
|
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.
If visualization is regenerated, will feedback be reset?
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, it will reset because the FeedbackThumbs component will be re-rendered.