T2viz UI metrics #312

ruanyl · 2024-09-14T05:57:55Z

Description

[Describe what this change achieves]

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test.
New functionality has user manual doc added.
Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2024-09-14T06:04:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.65%. Comparing base (4e68047) to head (ce386b6).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #312      +/-   ##
==========================================
+ Coverage   88.59%   88.65%   +0.06%     
==========================================
  Files          60       61       +1     
  Lines        1710     1728      +18     
  Branches      419      426       +7     
==========================================
+ Hits         1515     1532      +17     
- Misses        193      194       +1     
  Partials        2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chishui · 2024-09-14T07:29:28Z

public/components/visualization/text2viz.tsx

@@ -111,14 +127,23 @@ export const Text2Viz = () => {
          });
        } else {
          setEditorInput(JSON.stringify(result, undefined, 4));
+
+          // Report metric when visualization generated successfully
+          if (usageCollection) {


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

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?

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.

chishui · 2024-09-14T07:31:45Z

public/components/visualization/text2viz.tsx

@@ -410,6 +444,13 @@ export const Text2Viz = () => {
                  paddingSize="none"
                  scrollable={false}
                >
+                  {usageCollection ? (


IIUC, usageCollection is always available, the feature flag only controls whether it can send metrics to server.

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

chishui · 2024-09-14T07:33:47Z

public/components/feedback_thumbs.tsx

+}
+
+export const FeedbackThumbs = ({ usageCollection, appName, className }: Props) => {
+  const [feedback, setFeedback] = useState<'thumbs_up' | 'thumbs_down' | undefined>();


If visualization is regenerated, will feedback be reset?

Yes, it will reset because the FeedbackThumbs component will be re-rendered.

opensearch_dashboards.json

Signed-off-by: Yulong Ruan <[email protected]>

Hailong-am · 2024-09-27T09:39:14Z

public/components/visualization/text2viz.scss

+    right: 16px;
+    top: 4px;


nit: $euiSize is 16px

Hailong-am · 2024-09-27T09:43:41Z

public/components/visualization/text2viz.tsx

+          if (usageCollection) {
+            usageCollection.reportUiStats(
+              VIS_NLQ_APP_ID,
+              usageCollection.METRIC_TYPE.LOADED,
+              `saved-${uuidv4()}`
+            );


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.

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

I guess we don't need it any more?

You mean uuid or metric itself? We need both

Hailong-am · 2024-09-27T09:44:07Z

public/components/visualization/text2vega.ts

+        tap(() => this.status$.next('RUNNING')),
+        debounceTime(200)


any reason for changing the order?

Sometimes, it causes UI flashing.

* Add metrics for text2viz Signed-off-by: Yulong Ruan <[email protected]> * remove new line at the end of file Signed-off-by: Yulong Ruan <[email protected]> * fix imported types Signed-off-by: Yulong Ruan <[email protected]> * update changelogs Signed-off-by: Yulong Ruan <[email protected]> * fix UI flashing Signed-off-by: Yulong Ruan <[email protected]> * cleanup duplicate imports Signed-off-by: Yulong Ruan <[email protected]> --------- Signed-off-by: Yulong Ruan <[email protected]> (cherry picked from commit 519bb92) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* Add metrics for text2viz Signed-off-by: Yulong Ruan <[email protected]> * remove new line at the end of file Signed-off-by: Yulong Ruan <[email protected]> * fix imported types Signed-off-by: Yulong Ruan <[email protected]> * update changelogs Signed-off-by: Yulong Ruan <[email protected]> * fix UI flashing Signed-off-by: Yulong Ruan <[email protected]> * cleanup duplicate imports Signed-off-by: Yulong Ruan <[email protected]> --------- Signed-off-by: Yulong Ruan <[email protected]> (cherry picked from commit 519bb92) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

ruanyl requested review from lezzago, joshuali925, anirudha, SuZhou-Joe, xluo-aws, gaobinlong, Hailong-am, wanglam and raintygao as code owners September 14, 2024 05:57

chishui reviewed Sep 14, 2024

View reviewed changes

Hailong-am reviewed Sep 14, 2024

View reviewed changes

opensearch_dashboards.json Outdated Show resolved Hide resolved

ruanyl added the backport 2.x Trigger the backport flow to 2.x label Sep 18, 2024

Hailong-am mentioned this pull request Sep 19, 2024

Refactor for message action bar and add metrics for alerting summary #304

Merged

4 tasks

ruanyl force-pushed the t2viz-ui-metrics branch from 09b3aee to d6c41a1 Compare September 25, 2024 03:54

ruanyl added 5 commits September 27, 2024 15:03

Add metrics for text2viz

cde949f

Signed-off-by: Yulong Ruan <[email protected]>

remove new line at the end of file

4559e63

Signed-off-by: Yulong Ruan <[email protected]>

fix imported types

4f107e9

Signed-off-by: Yulong Ruan <[email protected]>

update changelogs

25442d1

Signed-off-by: Yulong Ruan <[email protected]>

fix UI flashing

9478056

Signed-off-by: Yulong Ruan <[email protected]>

ruanyl force-pushed the t2viz-ui-metrics branch from d6c41a1 to 9478056 Compare September 27, 2024 07:03

ruanyl added the t2viz label Sep 27, 2024

cleanup duplicate imports

c56f0d3

Signed-off-by: Yulong Ruan <[email protected]>

Hailong-am reviewed Sep 27, 2024

View reviewed changes

Hailong-am approved these changes Sep 27, 2024

View reviewed changes

ruanyl merged commit 519bb92 into opensearch-project:main Sep 27, 2024
9 checks passed

opensearch-trigger-bot bot mentioned this pull request Sep 27, 2024

[Backport 2.x] T2viz UI metrics #334

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T2viz UI metrics #312

T2viz UI metrics #312

ruanyl commented Sep 14, 2024

codecov bot commented Sep 14, 2024

chishui Sep 14, 2024

ruanyl Sep 14, 2024

chishui Sep 18, 2024

chishui Sep 14, 2024

ruanyl Sep 14, 2024

chishui Sep 14, 2024

ruanyl Sep 14, 2024 •

edited

Loading

Hailong-am Sep 27, 2024

Hailong-am Sep 27, 2024 •

edited

Loading

ruanyl Sep 27, 2024

chishui Sep 27, 2024

Hailong-am Sep 27, 2024

ruanyl Sep 27, 2024

T2viz UI metrics #312

T2viz UI metrics #312

Conversation

ruanyl commented Sep 14, 2024

Description

Issues Resolved

Check List

codecov bot commented Sep 14, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hailong-am Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl Sep 14, 2024 •

edited

Loading

Hailong-am Sep 27, 2024 •

edited

Loading