Add UI Metric for Discover 2.0 #8345

LDrago27 · 2024-09-25T20:38:51Z

Description

The PR aims at capturing following UI Metrics.

No of times discover has been loaded
No of times new discover has been loaded
No of times each datasource type queries have been executed
No of times each language queries have been executed

Issues Resolved

Screenshot

Testing the changes

Set usageCollection.uiMetric.enabled to true
Call the API /api/stats?extended=true&legacy=true&exclude_usage=false
Verify the ui_metric doesn't contain an entry corresponding to Discover
Navigate to Discover
Interact with Discover change the datasources, language
Wait for 90s
Call the API /api/stats?extended=true&legacy=true&exclude_usage=false and verify ui_metric contains an entry corresponding to Discover with the metrics corresponding to above interaction

Changelog

skip

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

github-actions · 2024-09-25T20:39:28Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

codecov · 2024-09-25T20:55:18Z

Codecov Report

Attention: Patch coverage is 54.05405% with 17 lines in your changes missing coverage. Please review.

Project coverage is 60.92%. Comparing base (33f4f64) to head (c6e407a).
Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
...gins/discover/public/ui_metric/report_ui_metric.ts	28.57%	10 Missing ⚠️
...ic/application/view_components/utils/use_search.ts	0.00%	3 Missing ⚠️
.../data_explorer/public/components/app_container.tsx	33.33%	1 Missing and 1 partial ⚠️
src/plugins/data_explorer/public/plugin.ts	0.00%	1 Missing ⚠️
...data_explorer/public/ui_metric/report_ui_metric.ts	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8345      +/-   ##
==========================================
- Coverage   60.95%   60.92%   -0.04%     
==========================================
  Files        3743     3756      +13     
  Lines       88857    89185     +328     
  Branches    13859    13941      +82     
==========================================
+ Hits        54164    54337     +173     
- Misses      31383    31467      +84     
- Partials     3310     3381      +71

Flag	Coverage Δ
Linux_1	`28.85% <37.50%> (+<0.01%)`	⬆️
Linux_2	`56.35% <ø> (?)`
Linux_3	`37.73% <54.05%> (-0.06%)`	⬇️
Linux_4	`29.96% <ø> (+0.02%)`	⬆️
Windows_1	`28.87% <37.50%> (+<0.01%)`	⬆️
Windows_2	`56.30% <ø> (+<0.01%)`	⬆️
Windows_3	`37.74% <54.05%> (-0.05%)`	⬇️
Windows_4	`29.96% <ø> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

src/plugins/data_explorer/public/services.ts

src/plugins/data_explorer/public/ui_metric/constants.ts

kavilla

couple nits and potential suggestion

kavilla · 2024-09-26T09:53:31Z

src/plugins/discover/public/application/view_components/utils/use_search.ts

@@ -33,6 +33,11 @@ import {
 } from '../../../opensearch_dashboards_services';
 import { SEARCH_ON_PAGE_LOAD_SETTING } from '../../../../common';
 import { syncQueryStateWithUrl } from '../../../../../data/public';
+import {
+  DATASOURCE_METRIC_SUFFIX,


super nit pick so feel free to ignore. i find it easier to have a const object that groups the constants so that i can import more with less.

for example.

METRIC_SUFFIX = { LANGUAGE: 'language_queries_count', DATA_SOURCE: 'data_source_queries_count' }

src/plugins/data_explorer/public/ui_metric/constants.ts

src/plugins/discover/public/application/view_components/utils/use_search.ts

kavilla · 2024-09-26T10:04:05Z

src/plugins/data_explorer/public/ui_metric/index.ts

appreciation comment for adding an index file

src/plugins/discover/public/ui_metric/constants.ts

src/plugins/discover/public/application/view_components/utils/use_search.ts

kavilla · 2024-09-26T23:23:01Z

src/plugins/discover/public/application/view_components/utils/use_search.ts

@@ -236,6 +241,19 @@ export const useSearch = (services: DiscoverViewServices) => {
          elapsedMs,
        },
      });
+
+      // Add tracking for datasource type and query if the query is not same as last query
+      const query = searchSource.getField('query');


can't remember if discover creates a new search source and it doesn't have a parent but we could also check if searchSource.parent.getField('query') . but then again i haven't checked so it parent might not ever be set for discover

src/plugins/data_explorer/public/components/app_container.tsx

kavilla · 2024-10-01T01:27:47Z

src/plugins/data_explorer/public/components/app_container.tsx

@@ -35,10 +42,34 @@ export const AppContainer = React.memo(

    const topLinkRef = useRef<HTMLDivElement>(null);
    const datePickerRef = useRef<HTMLDivElement>(null);
+
+    const initialQueryEnhancement = useRef(null); // Initialize to null


nit:

i think precedent has been to have a reference like

const isMounted = useRef(false);

and add a similar use hook to check if isMounted is true then we check if initialQueryEnhancement !== isEnhancementsEnabled. if false then we set initialQueryEnhancement.current to isEnhancementsEnabled. for consistency with other parts of the application

that said could we make the isEnhancementsEnabled a reference? and just do

isEnhancementsEnabled.current !== uiSettings?.get(QUERY_ENHANCEMENT_ENABLED_SETTING);

to reduce additional constants

src/plugins/discover/public/application/view_components/utils/use_search.ts

kavilla · 2024-10-01T01:32:52Z

src/plugins/discover/public/application/view_components/utils/use_search.ts

+      // Track the dataset type and language used
+
+      const query = searchSource.getField('query');
+      if (query && query.dataset?.type && query.language) {


just to verify, dataset with enhancements disabled will be undefined and therefore won't submit a ui metric. is that the current requirements or does it want to submit a ui metric for every language for every query

Its the former. Tracking dataset type and language are important only in the new discover

The requirements are specific to the new Discover experience , therefore in those cases where we don't have the datasets and language with enhancements off are not something we need to be worried about.

kavilla · 2024-10-01T01:38:49Z

src/plugins/discover/public/ui_metric/constants.ts

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export const DATASOURCE_METRIC_SUFFIX = 'data_source_queries_count';


i really think this should be dataset_queries_count or dataset_type_queries_count or data_queries_count to avoid confusion with the concept of a dataSource since a dataset can have a dataSource

ex:

the Index Pattern dataset type would make this count

INDEX_PATTERN_data_source_queries_count I think but as you can see here it has a data source:

index pattern datasource

s3 has a data source as well

so the ui metric being data source would cause confusion

we should also add a backlog item to migrate this portion over to the data plugin. eventually if the new discover ui becomes the standard and only or if other plugins on board to query enhancements then we can still get this insight on different types of dataset types and languages vs how it is used in discover

Updated the Metric to dataset_queries_count

src/plugins/discover/public/ui_metric/report_ui_metric.ts

src/plugins/data_explorer/public/services.ts

kavilla · 2024-10-01T02:16:34Z

src/plugins/data_explorer/public/ui_metric/constants.ts

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export const NEW_DISCOVER_LOAD_EVENT = 'new_discover_load_count';


super duper nit: don't change this PR specifically for this but the import line for src/plugins/data_explorer/public/components/app_container.tsx could be a smaller and not require future updates if we utilize an object. and easier to access for developers. and probably should keep the casing and the same.

suggestion like:

const APP_NAME = `discover`; const LOAD_EVENT = `load_count`; const OPT_IN_EVENT = `opt_in_count`; const OPT_OUT_EVENT = `opt_out_count`; export const METRIC = { DEFAULT: { LOAD_EVENT: `${APP_NAME}_${LOAD_EVENT}`, OPT_IN_EVENT: `${APP_NAME}_${OPT_IN_EVENT}`, OPT_OUT_EVENT: `${APP_NAME}_${OPT_OUT_EVENT}` }, LEGACY: { LOAD_EVENT: `${APP_NAME}_legacy_${LOAD_EVENT}`, OPT_IN_EVENT: `${APP_NAME}_legacy_${OPT_IN_EVENT}`, OPT_OUT_EVENT: `${APP_NAME}_legacy_${OPT_OUT_EVENT}` } }

then we can just import METRIC and access the constant with the dot notation.

some things to mention is that proposing using legacy instead of classifying vs using the new because with new or utilizing the 2.0 is that we version this ui metric to be 2.0 and then if we make the new discover the default then it becomes confusing for those who don't have context that the the default is the new ui metric and the previous version was the one without any classifying word on the ui metric. however, im not super tied to calling legacy. but i think we should go with legacy or new and avoid the 2.0 or 2.

also opted to ensure all the metrics are the same structure like

appname_classifier_event_metrictype

classifier being like new or legacy or nothing. this way if other plugins utilize the same standard then we have a nice way to aggregate on the results in a similar structure for visualizations and decision making. vs writing a query that filters out only the new and making the regex be something so it filters for new_discover or discover_new.

the final is using of interpolation this way if another developer wants to track another metric they can add by seeing the standard of using interpolation and do it as well. the reason why because i've seen people hardcode type constants over and over again and then there was a typo at which point we used that in key for local storage. but since the users used it already we are kind of stuck. so if we reduce the amount of opportunities for someone to make a typo then we could avoid an issue like queries failing to show up if the query was appName: discover but the developer added a new metric discoever_refresh_count where they mixed the e and the v in discover.

ashwin-pc

Lemme know your thoughts on the useEffect based tracking. The rest look good!

ashwin-pc · 2024-10-01T12:01:12Z

src/plugins/data/public/ui/index_pattern_select/index_pattern_select.test.tsx

@@ -49,7 +49,7 @@ describe('IndexPatternSelect', () => {

    bulkGetMock.mockResolvedValue({ savedObjects: [{ attributes: { title: 'test1' } }] });
    compInstance.debouncedFetch('');
-    await new Promise((resolve) => setTimeout(resolve, 300));
+    await new Promise((resolve) => setTimeout(resolve, 600));


Why this change?

This test case was a bit flakey in nature. hence to resolve it I had increased the timeout.

This is not the right way to deal with timers. Please revert this and i will deal with it in a separate PR.

ashwin-pc · 2024-10-01T12:42:28Z

src/plugins/data_explorer/public/components/app_container.tsx

+  DISCOVER_LOAD_EVENT,
+  NEW_DISCOVER_LOAD_EVENT,
+  NEW_DISCOVER_OPT_IN,
+  NEW_DISCOVER_OPT_OUT,


Why do we need all 4? New discover cant be triggered without a page refresh. So the useEffect based tracking should be unnecessary.

Oh. Then it might not make sense to add them in here. Removing the OPT_IN and OPT_OUT tracking from here.

ashwin-pc · 2024-10-01T12:47:54Z

src/plugins/discover/public/application/view_components/utils/use_search.ts

+      // Track the dataset type and language used
+
+      const query = searchSource.getField('query');
+      if (query && query.dataset?.type && query.language) {


Its the former. Tracking dataset type and language are important only in the new discover

Signed-off-by: Suchit Sahoo <[email protected]>

sejli

+1 to @kavilla's nonblocking comment on the string constants, might need that documented somewhere as part of OSD best practices. Otherwise, LGTM.

Signed-off-by: Suchit Sahoo <[email protected]> (cherry picked from commit daa59de) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…project#8427)

github-actions bot added the all-star-contributor label Sep 25, 2024

github-actions bot added the failed changeset label Sep 25, 2024

LDrago27 removed the failed changeset label Sep 25, 2024

github-actions bot added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Sep 25, 2024

LDrago27 force-pushed the discoverUiMetric branch from a68dcb5 to dc9e9ef Compare September 25, 2024 23:31

LDrago27 marked this pull request as ready for review September 25, 2024 23:36

LDrago27 requested a review from huyaboo as a code owner September 25, 2024 23:36

LDrago27 added discover for discover reinvent backport 2.x v2.18.0 labels Sep 25, 2024

kavilla reviewed Sep 26, 2024

View reviewed changes

src/plugins/data_explorer/public/services.ts Show resolved Hide resolved

kavilla reviewed Sep 26, 2024

View reviewed changes

src/plugins/data_explorer/public/ui_metric/constants.ts Outdated Show resolved Hide resolved

kavilla reviewed Sep 26, 2024

View reviewed changes

src/plugins/discover/public/ui_metric/constants.ts Outdated Show resolved Hide resolved

LDrago27 force-pushed the discoverUiMetric branch 2 times, most recently from 97e3456 to 792dff7 Compare September 26, 2024 19:07

LDrago27 added discover-next and removed v2.18.0 labels Sep 26, 2024

AMoo-Miki added the v2.18.0 label Sep 26, 2024

LDrago27 force-pushed the discoverUiMetric branch 2 times, most recently from 81c30aa to 299c541 Compare September 26, 2024 21:23

kavilla reviewed Sep 26, 2024

View reviewed changes

LDrago27 force-pushed the discoverUiMetric branch 2 times, most recently from 244244d to 2e3e574 Compare September 30, 2024 21:32

kavilla reviewed Oct 1, 2024

View reviewed changes

ashwin-pc reviewed Oct 1, 2024

View reviewed changes

Add Ui Metric for Discover 2.0

c6e407a

Signed-off-by: Suchit Sahoo <[email protected]>

LDrago27 force-pushed the discoverUiMetric branch from 2e3e574 to c6e407a Compare October 1, 2024 17:43

ashwin-pc approved these changes Oct 1, 2024

View reviewed changes

sejli approved these changes Oct 1, 2024

View reviewed changes

LDrago27 merged commit daa59de into opensearch-project:main Oct 1, 2024
65 of 67 checks passed

opensearch-trigger-bot bot mentioned this pull request Oct 1, 2024

[Backport 2.x] Add UI Metric for Discover 2.0 #8427

Merged

LDrago27 pushed a commit that referenced this pull request Oct 2, 2024

Add Ui Metric for Discover 2.0 (#8345) (#8427)

c0711ac

SuZhou-Joe pushed a commit to SuZhou-Joe/OpenSearch-Dashboards that referenced this pull request Oct 3, 2024

Add Ui Metric for Discover 2.0 (opensearch-project#8345) (opensearch-…

27d2306

…project#8427)

ashwin-pc mentioned this pull request Oct 18, 2024

[RFC] Create a plugin for metric reporting #8625

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI Metric for Discover 2.0 #8345

Add UI Metric for Discover 2.0 #8345

LDrago27 commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024

codecov bot commented Sep 25, 2024 •

edited

Loading

kavilla left a comment

kavilla Sep 26, 2024

kavilla Sep 26, 2024

kavilla Sep 26, 2024

kavilla Oct 1, 2024

kavilla Oct 1, 2024

ashwin-pc Oct 1, 2024

LDrago27 Oct 1, 2024

kavilla Oct 1, 2024

kavilla Oct 1, 2024

LDrago27 Oct 1, 2024

kavilla Oct 1, 2024

ashwin-pc left a comment

ashwin-pc Oct 1, 2024

LDrago27 Oct 1, 2024

AMoo-Miki Oct 1, 2024 •

edited

Loading

ashwin-pc Oct 1, 2024

LDrago27 Oct 1, 2024 •

edited

Loading

ashwin-pc Oct 1, 2024

sejli left a comment

Add UI Metric for Discover 2.0 #8345

Add UI Metric for Discover 2.0 #8345

Conversation

LDrago27 commented Sep 25, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Sep 25, 2024

❌ Empty Changelog Section

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

kavilla left a 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

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

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AMoo-Miki Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LDrago27 Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sejli left a comment

Choose a reason for hiding this comment

LDrago27 commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

AMoo-Miki Oct 1, 2024 •

edited

Loading

LDrago27 Oct 1, 2024 •

edited

Loading