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

T2viz UI metrics #312

Merged
merged 6 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- fix: incorrect string escaping of vega schema([325](https://github.com/opensearch-project/dashboards-assistant/pull/325))
- feat: register the AI actions to query controls in discover([#327](https://github.com/opensearch-project/dashboards-assistant/pull/327))
- fix: t2viz ux improvements([#330](https://github.com/opensearch-project/dashboards-assistant/pull/330))
- feat: report metrics for text to visualization([#312](https://github.com/opensearch-project/dashboards-assistant/pull/312))

### 📈 Features/Enhancements

Expand Down
79 changes: 79 additions & 0 deletions public/components/feedback_thumbs.test.tsx
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();
});
});
59 changes: 59 additions & 0 deletions public/components/feedback_thumbs.tsx
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>();
Copy link
Contributor

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?

Copy link
Member Author

@ruanyl ruanyl Sep 14, 2024

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.


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>
);
};
4 changes: 2 additions & 2 deletions public/components/visualization/text2vega.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason for changing the order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes, it causes UI flashing.

)
.pipe(
switchMap((v) =>
Expand Down
7 changes: 7 additions & 0 deletions public/components/visualization/text2viz.scss
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,11 @@
padding-top: 15px;
padding-left: 30px;
}

.feedback_thumbs {
position: absolute;
right: 16px;
top: 4px;
Comment on lines +22 to +23
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: $euiSize is 16px

z-index: 9999;
}
}
47 changes: 44 additions & 3 deletions public/components/visualization/text2viz.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 }>();
Expand All @@ -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('');
Expand Down Expand Up @@ -112,14 +128,23 @@ export const Text2Viz = () => {
});
} else {
setEditorInput(JSON.stringify(result, undefined, 4));

// Report metric when visualization generated successfully
if (usageCollection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use METRIC_TYPE.COUNT to either record a 1 for success or 0 for failure. We could calculate average to get success rate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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

try {
 const response = await fetch();
emitMetric("fetch_success", COUNT, 1);
} catch( err: any) {
emitMetric("fetch_success", COUNT, 0);
}

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
Expand Down Expand Up @@ -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
Copy link
Collaborator

@Hailong-am Hailong-am Sep 27, 2024

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we don't need it any more? @chishui any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we don't need it any more?

You mean uuid or metric itself? We need both

}
}
} catch (e) {
notifications.toasts.addDanger({
Expand Down Expand Up @@ -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', {
Expand Down Expand Up @@ -412,6 +446,13 @@ export const Text2Viz = () => {
paddingSize="none"
scrollable={false}
>
{usageCollection ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

usageCollection is declared as an optional plugin of dashboards-assistant plugin. Theoretically, it can be undefined.

<FeedbackThumbs
usageCollection={usageCollection}
appName={VIS_NLQ_APP_ID}
className="feedback_thumbs"
/>
) : null}
<EmbeddableRenderer factory={factory} input={visInput} />
</EuiResizablePanel>
<EuiResizableButton />
Expand Down
6 changes: 5 additions & 1 deletion public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@ import { AssistantClient } from './services/assistant_client';
import { UiActionsSetup, UiActionsStart } from '../../../src/plugins/ui_actions/public';
import { ExpressionsSetup, ExpressionsStart } from '../../../src/plugins/expressions/public';
import { SavedObjectsStart } from '../../../src/plugins/saved_objects/public';
import {
UsageCollectionStart,
UsageCollectionSetup,
} from '../../../src/plugins/usage_collection/public';

import { UsageCollectionSetup } from '../../../src/plugins/usage_collection/public';
import { ConfigSchema } from '../common/types/config';

export interface RenderProps {
Expand Down Expand Up @@ -49,6 +52,7 @@ export interface AssistantPluginStartDependencies {
uiActions: UiActionsStart;
expressions: ExpressionsStart;
savedObjects: SavedObjectsStart;
usageCollection?: UsageCollectionStart;
}

export interface AssistantPluginSetupDependencies {
Expand Down
Loading