Address review comments

Signed-off-by: Siddhant Deshmukh <[email protected]>
opensearch-project · Jan 13, 2025 · 956c0ff · 956c0ff
1 parent 7336988
commit 956c0ff
Show file tree

Hide file tree

Showing 8 changed files with 95 additions and 65 deletions.
diff --git a/common/constants.ts b/common/constants.ts
@@ -5,7 +5,7 @@
 
 export const TIMESTAMP = 'Timestamp';
 export const TYPE = 'Type';
-export const QUERY_HASHCODE = 'Query Hashcode';
+export const QUERY_GROUP_HASHCODE = 'Query Hashcode';
 export const QUERY_COUNT = 'Query Count';
 export const LATENCY = 'Latency';
 export const CPU_TIME = 'CPU Time';
@@ -15,3 +15,6 @@ export const SEARCH_TYPE = 'Search type';
 export const NODE_ID = 'Coordinator node ID';
 export const TOTAL_SHARDS = 'Total shards';
 export const GROUP_BY = 'Group by';
+export const AVERAGE_LATENCY = 'Average Latency';
+export const AVERAGE_CPU_TIME = 'Average CPU Time';
+export const AVERAGE_MEMORY_USAGE = 'Average Memory Usage';
diff --git a/public/components/app.test.tsx b/public/components/app.test.tsx
@@ -8,7 +8,6 @@ import { render } from '@testing-library/react';
 import { coreMock } from '../../../../src/core/public/mocks';
 import { MemoryRouter as Router } from 'react-router-dom';
 import { QueryInsightsDashboardsApp } from './app';
-import '@testing-library/jest-dom/extend-expect';
 
 describe('<QueryInsightsDashboardsApp /> spec', () => {
   it('renders the component', () => {

diff --git a/public/pages/QueryDetails/Components/QuerySummary.tsx b/public/pages/QueryDetails/Components/QuerySummary.tsx
@@ -16,6 +16,7 @@ import {
   TIMESTAMP,
   TOTAL_SHARDS,
 } from '../../../../common/constants';
+import { calculateMetric } from '../../Utils/MetricUtils';
 
 // Panel component for displaying query detail values
 const PanelItem = ({ label, value }: { label: string; value: string | number }) => (
@@ -46,23 +47,25 @@ const QuerySummary = ({ query }: { query: SearchQueryRecord }) => {
         <PanelItem
           label={LATENCY}
           value={
-            measurements.latency?.number !== undefined && measurements.latency?.count !== undefined
-              ? `${(measurements.latency.number / measurements.latency.count).toFixed(2)} ms`
-              : 'N/A'
+            calculateMetric(measurements.latency?.number, measurements.latency?.count) === 'N/A'
+              ? 'N/A'
+              : `${calculateMetric(measurements.latency?.number, measurements.latency?.count)} ms`
           }
         />
         <PanelItem
           label={CPU_TIME}
           value={
-            measurements.cpu?.number !== undefined && measurements.cpu?.count !== undefined
-              ? `${(measurements.cpu.number / measurements.cpu.count / 1000000).toFixed(2)} ms`
-              : 'N/A'
+            calculateMetric(measurements.cpu?.number, measurements.cpu?.count, 1000000) === 'N/A'
+              ? 'N/A'
+              : `${calculateMetric(measurements.cpu?.number, measurements.cpu?.count, 1000000)} ms`
           }
         />
         <PanelItem
           label={MEMORY_USAGE}
           value={
-            measurements.memory?.number !== undefined ? `${measurements.memory.number} B` : 'N/A'
+            calculateMetric(measurements.memory?.number, measurements.memory?.count) === 'N/A'
+              ? 'N/A'
+              : `${calculateMetric(measurements.memory?.number, measurements.memory?.count)} B`
           }
         />
         <PanelItem label={INDICES} value={indices.toString()} />

diff --git a/public/pages/QueryGroupDetails/Components/QueryGroupAggregateSummary.test.tsx b/public/pages/QueryGroupDetails/Components/QueryGroupAggregateSummary.test.tsx
@@ -24,9 +24,9 @@ describe('QueryGroupAggregateSummary', () => {
 
     expect(screen.getByText('Aggregate summary for 8 queries')).toBeInTheDocument();
     expect(screen.getByText('Query Hashcode')).toBeInTheDocument();
-    expect(screen.getByText('Latency')).toBeInTheDocument();
-    expect(screen.getByText('CPU Time')).toBeInTheDocument();
-    expect(screen.getByText('Memory Usage')).toBeInTheDocument();
+    expect(screen.getByText('Average Latency')).toBeInTheDocument();
+    expect(screen.getByText('Average CPU Time')).toBeInTheDocument();
+    expect(screen.getByText('Average Memory Usage')).toBeInTheDocument();
     expect(screen.getByText('Group by')).toBeInTheDocument();
   });
 
@@ -65,7 +65,7 @@ describe('QueryGroupAggregateSummary', () => {
       </MemoryRouter>
     );
 
-    const memoryUsage = '132224 B';
+    const memoryUsage = '16528.00 B';
     expect(screen.getByText(memoryUsage)).toBeInTheDocument();
   });
 

diff --git a/public/pages/QueryGroupDetails/Components/QueryGroupAggregateSummary.tsx b/public/pages/QueryGroupDetails/Components/QueryGroupAggregateSummary.tsx
@@ -6,12 +6,13 @@
 import React from 'react';
 import { EuiFlexGrid, EuiFlexItem, EuiHorizontalRule, EuiPanel, EuiText } from '@elastic/eui';
 import {
-  CPU_TIME,
+  AVERAGE_CPU_TIME,
+  AVERAGE_LATENCY,
+  AVERAGE_MEMORY_USAGE,
   GROUP_BY,
-  LATENCY,
-  MEMORY_USAGE,
-  QUERY_HASHCODE,
+  QUERY_GROUP_HASHCODE,
 } from '../../../../common/constants';
+import { calculateMetric } from '../../Utils/MetricUtils';
 
 // Panel component for displaying query group detail values
 const PanelItem = ({ label, value }: { label: string; value: string | number }) => (
@@ -24,7 +25,7 @@ const PanelItem = ({ label, value }: { label: string; value: string | number })
 );
 
 export const QueryGroupAggregateSummary = ({ query }: { query: any }) => {
-  const { measurements, query_hashcode: queryHashcode, group_by: groupBy } = query;
+  const { measurements, query_hashcode: queryGroupHashcode, group_by: groupBy } = query;
   const queryCount =
     measurements.latency?.count || measurements.cpu?.count || measurements.memory?.count || 1;
   return (
@@ -36,27 +37,33 @@ export const QueryGroupAggregateSummary = ({ query }: { query: any }) => {
       </EuiText>
       <EuiHorizontalRule margin="m" />
       <EuiFlexGrid columns={4}>
-        <PanelItem label={QUERY_HASHCODE} value={queryHashcode} />
+        <PanelItem label={QUERY_GROUP_HASHCODE} value={queryGroupHashcode} />
         <PanelItem
-          label={LATENCY}
+          label={AVERAGE_LATENCY}
           value={
-            measurements.latency?.number !== undefined && measurements.latency?.count !== undefined
-              ? `${(measurements.latency.number / measurements.latency.count).toFixed(2)} ms`
-              : 'N/A'
+            calculateMetric(measurements.latency?.number, measurements.latency?.count, 1) === 'N/A'
+              ? 'N/A'
+              : `${calculateMetric(
+                  measurements.latency?.number,
+                  measurements.latency?.count,
+                  1
+                )} ms`
           }
         />
         <PanelItem
-          label={CPU_TIME}
+          label={AVERAGE_CPU_TIME}
           value={
-            measurements.cpu?.number !== undefined && measurements.cpu?.count !== undefined
-              ? `${(measurements.cpu.number / measurements.cpu.count / 1000000).toFixed(2)} ms`
-              : 'N/A'
+            calculateMetric(measurements.cpu?.number, measurements.cpu?.count, 1000000) === 'N/A'
+              ? 'N/A'
+              : `${calculateMetric(measurements.cpu?.number, measurements.cpu?.count, 1000000)} ms`
           }
         />
         <PanelItem
-          label={MEMORY_USAGE}
+          label={AVERAGE_MEMORY_USAGE}
           value={
-            measurements.memory?.number !== undefined ? `${measurements.memory.number} B` : 'N/A'
+            calculateMetric(measurements.memory?.number, measurements.memory?.count, 1) === 'N/A'
+              ? 'N/A'
+              : `${calculateMetric(measurements.memory?.number, measurements.memory?.count, 1)} B`
           }
         />
         <PanelItem label={GROUP_BY} value={groupBy !== undefined ? `${groupBy}` : 'N/A'} />

diff --git a/public/pages/QueryInsights/QueryInsights.tsx b/public/pages/QueryInsights/QueryInsights.tsx
@@ -17,12 +17,13 @@ import {
   MEMORY_USAGE,
   NODE_ID,
   QUERY_COUNT,
-  QUERY_HASHCODE,
+  QUERY_GROUP_HASHCODE,
   SEARCH_TYPE,
   TIMESTAMP,
   TOTAL_SHARDS,
   TYPE,
 } from '../../../common/constants';
+import { calculateMetric } from '../Utils/MetricUtils';
 
 const TIMESTAMP_FIELD = 'timestamp';
 const MEASUREMENTS_FIELD = 'measurements';
@@ -75,8 +76,8 @@ const QueryInsights = ({
 
   const cols: Array<EuiBasicTableColumn<any>> = [
     {
-      name: QUERY_HASHCODE,
-      render: (query: any) => {
+      name: QUERY_GROUP_HASHCODE,
+      render: (query: SearchQueryRecord) => {
         return (
           <span>
             <EuiLink
@@ -88,17 +89,18 @@ const QueryInsights = ({
                 history.push(route);
               }}
             >
-              {query.query_hashcode || '-'}
+              {query.query_group_hashcode || '-'}{' '}
+              {/* TODO: Remove fallback '-' once query_id is available - #159 */}
             </EuiLink>
           </span>
         );
       },
-      sortable: (query) => query.query_hashcode || '-',
+      sortable: (query: SearchQueryRecord) => query.query_group_hashcode || '-',
       truncateText: true,
     },
     {
       name: TYPE,
-      render: (query: any) => {
+      render: (query: SearchQueryRecord) => {
         return (
           <span>
             <EuiLink
@@ -121,14 +123,14 @@ const QueryInsights = ({
     {
       field: MEASUREMENTS_FIELD,
       name: QUERY_COUNT,
-      render: (measurements: any) =>
+      render: (measurements: SearchQueryRecord['measurements']) =>
         `${
           measurements?.latency?.count ||
           measurements?.cpu?.count ||
           measurements?.memory?.count ||
           1
         }`,
-      sortable: (measurements: any) => {
+      sortable: (measurements: SearchQueryRecord['measurements']) => {
         return Number(
           measurements?.latency?.count ||
             measurements?.cpu?.count ||
@@ -141,7 +143,7 @@ const QueryInsights = ({
     {
       // Make into flyout instead?
       name: TIMESTAMP,
-      render: (query: any) => {
+      render: (query: SearchQueryRecord) => {
         const isQuery = query.group_by === 'NONE';
         const linkContent = isQuery ? convertTime(query.timestamp) : '-';
         const onClickHandler = () => history.push(`/query-details/${hash(query)}`);
@@ -158,13 +160,13 @@ const QueryInsights = ({
     {
       field: MEASUREMENTS_FIELD,
       name: LATENCY,
-      render: (measurements: any) => {
-        const latencyValue = measurements?.latency?.number;
-        const latencyCount = measurements?.latency?.count;
-        const result =
-          latencyValue !== undefined && latencyCount !== undefined
-            ? (latencyValue / latencyCount).toFixed(2)
-            : METRIC_DEFAULT_MSG;
+      render: (measurements: SearchQueryRecord['measurements']) => {
+        const result = calculateMetric(
+          measurements?.latency?.number,
+          measurements?.latency?.count,
+          1,
+          METRIC_DEFAULT_MSG
+        );
         return `${result} ms`;
       },
       sortable: true,
@@ -173,13 +175,13 @@ const QueryInsights = ({
     {
       field: MEASUREMENTS_FIELD,
       name: CPU_TIME,
-      render: (measurements: any) => {
-        const cpuValue = measurements?.cpu?.number;
-        const cpuCount = measurements?.cpu?.count;
-        const result =
-          cpuValue !== undefined && cpuCount !== undefined
-            ? (cpuValue / cpuCount / 1000000).toFixed(2)
-            : METRIC_DEFAULT_MSG;
+      render: (measurements: SearchQueryRecord['measurements']) => {
+        const result = calculateMetric(
+          measurements?.cpu?.number,
+          measurements?.cpu?.count,
+          1000000, // Divide by 1,000,000 for CPU time
+          METRIC_DEFAULT_MSG
+        );
         return `${result} ms`;
       },
       sortable: true,
@@ -188,13 +190,13 @@ const QueryInsights = ({
     {
       field: MEASUREMENTS_FIELD,
       name: MEMORY_USAGE,
-      render: (measurements: any) => {
-        const memoryValue = measurements?.memory?.number;
-        const memoryCount = measurements?.memory?.count;
-        const result =
-          memoryValue !== undefined && memoryCount !== undefined
-            ? (memoryValue / memoryCount).toFixed(2)
-            : METRIC_DEFAULT_MSG;
+      render: (measurements: SearchQueryRecord['measurements']) => {
+        const result = calculateMetric(
+          measurements?.memory?.number,
+          measurements?.memory?.count,
+          1,
+          METRIC_DEFAULT_MSG
+        );
         return `${result} B`;
       },
       sortable: true,
@@ -203,7 +205,7 @@ const QueryInsights = ({
     {
       field: INDICES_FIELD,
       name: INDICES,
-      render: (indices: string[], query: any) => {
+      render: (indices: string[], query: SearchQueryRecord) => {
         const isSimilarity = query.group_by === 'SIMILARITY';
         return isSimilarity ? '-' : Array.from(new Set(indices.flat())).join(', ');
       },
@@ -213,7 +215,7 @@ const QueryInsights = ({
     {
       field: SEARCH_TYPE_FIELD,
       name: SEARCH_TYPE,
-      render: (searchType: string, query: any) => {
+      render: (searchType: string, query: SearchQueryRecord) => {
         const isSimilarity = query.group_by === 'SIMILARITY';
         return isSimilarity ? '-' : searchType.replaceAll('_', ' ');
       },
@@ -223,7 +225,7 @@ const QueryInsights = ({
     {
       field: NODE_ID_FIELD,
       name: NODE_ID,
-      render: (nodeId: string, query: any) => {
+      render: (nodeId: string, query: SearchQueryRecord) => {
         const isSimilarity = query.group_by === 'SIMILARITY';
         return isSimilarity ? '-' : nodeId;
       },
@@ -233,7 +235,7 @@ const QueryInsights = ({
     {
       field: TOTAL_SHARDS_FIELD,
       name: TOTAL_SHARDS,
-      render: (totalShards: number, query: any) => {
+      render: (totalShards: number, query: SearchQueryRecord) => {
         const isSimilarity = query.group_by === 'SIMILARITY';
         return isSimilarity ? '-' : totalShards;
       },
@@ -354,7 +356,7 @@ const QueryInsights = ({
         ],
       }}
       allowNeutralSort={false}
-      itemId={(query) => `${query.query_hashcode}-${query.timestamp}`}
+      itemId={(query) => `${query.query_group_hashcode}-${query.timestamp}`}
     />
   );
 };

diff --git a/public/pages/Utils/MetricUtils.ts b/public/pages/Utils/MetricUtils.ts
@@ -0,0 +1,16 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export function calculateMetric(
+  value?: number,
+  count?: number,
+  factor: number = 1,
+  defaultMsg: string = 'N/A'
+): string {
+  if (value !== undefined && count !== undefined) {
+    return (value / count / factor).toFixed(2);
+  }
+  return defaultMsg;
+}
diff --git a/types/types.ts b/types/types.ts
@@ -20,7 +20,7 @@ export interface SearchQueryRecord {
   indices: string[];
   phase_latency_map: PhaseLatencyMap;
   task_resource_usages: Task[];
-  query_hashcode: string;
+  query_group_hashcode: string;
   group_by: string;
 }