Tracing: Next/prev text for span filters (#67208)

* Next/prev text

* Tests

* Update test

* Updated state vars and also made other improvements

public/app/features/explore/TraceView/TraceView.tsx

@@ -166,7 +166,6 @@ export function TraceView(props: Props) {
                 setShowSpanFilters={setShowSpanFilters}
                 showSpanFilterMatchesOnly={showSpanFilterMatchesOnly}
                 setShowSpanFilterMatchesOnly={setShowSpanFilterMatchesOnly}
-                focusedSpanIdForSearch={newTraceViewHeaderFocusedSpanIdForSearch}
                 setFocusedSpanIdForSearch={setNewTraceViewHeaderFocusedSpanIdForSearch}
                 spanFilterMatches={spanFilterMatches}
                 datasourceType={datasourceType}

public/app/features/explore/TraceView/TraceViewContainer.test.tsx

@@ -134,6 +134,56 @@ describe('TraceViewContainer', () => {
     ).toContain('rowFocused');
   });
 
+  it('can select next/prev results', async () => {
+    config.featureToggles.newTraceViewHeader = true;
+    renderTraceViewContainer();
+    const spanFiltersButton = screen.getByRole('button', { name: 'Span Filters' });
+    await user.click(spanFiltersButton);
+
+    const nextResultButton = screen.getByRole('button', { name: 'Next result button' });
+    const prevResultButton = screen.getByRole('button', { name: 'Prev result button' });
+    expect((nextResultButton as HTMLButtonElement)['disabled']).toBe(true);
+    expect((prevResultButton as HTMLButtonElement)['disabled']).toBe(true);
+
+    await user.click(screen.getByLabelText('Select tag key'));
+    const tagOption = screen.getByText('component');
+    await waitFor(() => expect(tagOption).toBeInTheDocument());
+    await user.click(tagOption);
+
+    await waitFor(() => {
+      expect(
+        screen.queryAllByText('', { selector: 'div[data-testid="span-view"]' })[0].parentElement!.className
+      ).toContain('rowMatchingFilter');
+      expect(
+        screen.queryAllByText('', { selector: 'div[data-testid="span-view"]' })[1].parentElement!.className
+      ).toContain('rowMatchingFilter');
+      expect(
+        screen.queryAllByText('', { selector: 'div[data-testid="span-view"]' })[2].parentElement!.className
+      ).toContain('rowMatchingFilter');
+    });
+
+    expect((nextResultButton as HTMLButtonElement)['disabled']).toBe(false);
+    expect((prevResultButton as HTMLButtonElement)['disabled']).toBe(false);
+    await user.click(nextResultButton);
+    await waitFor(() => {
+      expect(
+        screen.queryAllByText('', { selector: 'div[data-testid="span-view"]' })[0].parentElement!.className
+      ).toContain('rowFocused');
+    });
+    await user.click(nextResultButton);
+    await waitFor(() => {
+      expect(
+        screen.queryAllByText('', { selector: 'div[data-testid="span-view"]' })[1].parentElement!.className
+      ).toContain('rowFocused');
+    });
+    await user.click(prevResultButton);
+    await waitFor(() => {
+      expect(
+        screen.queryAllByText('', { selector: 'div[data-testid="span-view"]' })[0].parentElement!.className
+      ).toContain('rowFocused');
+    });
+  });
+
   it('show matches only works as expected', async () => {
     config.featureToggles.newTraceViewHeader = true;
     renderTraceViewContainer();

public/app/features/explore/TraceView/components/TracePageHeader/NewTracePageHeader.test.tsx

@@ -33,7 +33,6 @@ const setup = () => {
     showSpanFilterMatchesOnly: false,
     setShowSpanFilterMatchesOnly: jest.fn(),
     spanFilterMatches: undefined,
-    focusedSpanIdForSearch: '',
     setFocusedSpanIdForSearch: jest.fn(),
     datasourceType: 'tempo',
     setHeaderHeight: jest.fn(),

public/app/features/explore/TraceView/components/TracePageHeader/NewTracePageHeader.tsx

@@ -41,7 +41,6 @@ export type TracePageHeaderProps = {
   setShowSpanFilters: (isOpen: boolean) => void;
   showSpanFilterMatchesOnly: boolean;
   setShowSpanFilterMatchesOnly: (showMatchesOnly: boolean) => void;
-  focusedSpanIdForSearch: string;
   setFocusedSpanIdForSearch: React.Dispatch<React.SetStateAction<string>>;
   spanFilterMatches: Set<string> | undefined;
   datasourceType: string;
@@ -58,7 +57,6 @@ export const NewTracePageHeader = memo((props: TracePageHeaderProps) => {
     setShowSpanFilters,
     showSpanFilterMatchesOnly,
     setShowSpanFilterMatchesOnly,
-    focusedSpanIdForSearch,
     setFocusedSpanIdForSearch,
     spanFilterMatches,
     datasourceType,
@@ -140,7 +138,6 @@ export const NewTracePageHeader = memo((props: TracePageHeaderProps) => {
         search={search}
         setSearch={setSearch}
         spanFilterMatches={spanFilterMatches}
-        focusedSpanIdForSearch={focusedSpanIdForSearch}
         setFocusedSpanIdForSearch={setFocusedSpanIdForSearch}
         datasourceType={datasourceType}
       />

public/app/features/explore/TraceView/components/TracePageHeader/NewTracePageSearchBar.test.tsx

@@ -12,25 +12,49 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-import { render, screen } from '@testing-library/react';
+import { render, screen, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 import React from 'react';
 
 import { defaultFilters } from '../../useSearch';
 
-import NewTracePageSearchBar, { TracePageSearchBarProps } from './NewTracePageSearchBar';
-
-const defaultProps = {
-  search: defaultFilters,
-  setFocusedSpanIdForSearch: jest.fn(),
-  showSpanFilterMatchesOnly: false,
-  setShowSpanFilterMatchesOnly: jest.fn(),
-};
+import NewTracePageSearchBar, { getStyles } from './NewTracePageSearchBar';
 
 describe('<NewTracePageSearchBar>', () => {
+  let user: ReturnType<typeof userEvent.setup>;
+  beforeEach(() => {
+    jest.useFakeTimers();
+    // Need to use delay: null here to work with fakeTimers
+    // see https://github.com/testing-library/user-event/issues/833
+    user = userEvent.setup({ delay: null });
+  });
+  afterEach(() => {
+    jest.useRealTimers();
+  });
+
+  const NewTracePageSearchBarWithProps = (props: { matches: string[] | undefined }) => {
+    const searchBarProps = {
+      search: defaultFilters,
+      spanFilterMatches: props.matches ? new Set(props.matches) : undefined,
+      showSpanFilterMatchesOnly: false,
+      setShowSpanFilterMatchesOnly: jest.fn(),
+      setFocusedSpanIdForSearch: jest.fn(),
+      datasourceType: '',
+      reset: jest.fn(),
+      totalSpans: 100,
+    };
+
+    return <NewTracePageSearchBar {...searchBarProps} />;
+  };
+
+  it('should render', () => {
+    expect(() => render(<NewTracePageSearchBarWithProps matches={[]} />)).not.toThrow();
+  });
+
   it('renders buttons', () => {
-    render(<NewTracePageSearchBar {...(defaultProps as unknown as TracePageSearchBarProps)} />);
-    const nextResButton = screen.getByRole('button', { name: 'Next result button' });
-    const prevResButton = screen.getByRole('button', { name: 'Prev result button' });
+    render(<NewTracePageSearchBarWithProps matches={[]} />);
+    const nextResButton = screen.queryByRole('button', { name: 'Next result button' });
+    const prevResButton = screen.queryByRole('button', { name: 'Prev result button' });
     const resetFiltersButton = screen.getByRole('button', { name: 'Reset filters button' });
     expect(nextResButton).toBeInTheDocument();
     expect(prevResButton).toBeInTheDocument();
@@ -40,22 +64,55 @@ describe('<NewTracePageSearchBar>', () => {
     expect((resetFiltersButton as HTMLButtonElement)['disabled']).toBe(true);
   });
 
-  it('renders buttons that can be used to search if results found', () => {
-    const props = {
-      ...defaultProps,
-      spanFilterMatches: new Set(['2ed38015486087ca']),
-    };
-    render(<NewTracePageSearchBar {...(props as unknown as TracePageSearchBarProps)} />);
-    const nextResButton = screen.getByRole('button', { name: 'Next result button' });
-    const prevResButton = screen.getByRole('button', { name: 'Prev result button' });
+  it('renders total spans', async () => {
+    render(<NewTracePageSearchBarWithProps matches={undefined} />);
+    expect(screen.getByText('100 spans')).toBeDefined();
+  });
+
+  it('renders buttons that can be used to search if filters added', () => {
+    render(<NewTracePageSearchBarWithProps matches={['2ed38015486087ca']} />);
+    const nextResButton = screen.queryByRole('button', { name: 'Next result button' });
+    const prevResButton = screen.queryByRole('button', { name: 'Prev result button' });
     expect(nextResButton).toBeInTheDocument();
     expect(prevResButton).toBeInTheDocument();
     expect((nextResButton as HTMLButtonElement)['disabled']).toBe(false);
     expect((prevResButton as HTMLButtonElement)['disabled']).toBe(false);
+    expect(screen.getByText('1 match')).toBeDefined();
+  });
+
+  it('renders correctly when moving through matches', async () => {
+    render(<NewTracePageSearchBarWithProps matches={['1ed38015486087ca', '2ed38015486087ca', '3ed38015486087ca']} />);
+    const nextResButton = screen.queryByRole('button', { name: 'Next result button' });
+    const prevResButton = screen.queryByRole('button', { name: 'Prev result button' });
+    expect(screen.getByText('3 matches')).toBeDefined();
+    await user.click(nextResButton!);
+    expect(screen.getByText('1/3 matches')).toBeDefined();
+    await user.click(nextResButton!);
+    expect(screen.getByText('2/3 matches')).toBeDefined();
+    await user.click(nextResButton!);
+    expect(screen.getByText('3/3 matches')).toBeDefined();
+    await user.click(nextResButton!);
+    expect(screen.getByText('1/3 matches')).toBeDefined();
+    await user.click(prevResButton!);
+    expect(screen.getByText('3/3 matches')).toBeDefined();
+    await user.click(prevResButton!);
+    expect(screen.getByText('2/3 matches')).toBeDefined();
+  });
+
+  it('renders correctly when there are no matches i.e. too many filters added', async () => {
+    const { container } = render(<NewTracePageSearchBarWithProps matches={[]} />);
+    const styles = getStyles();
+    const tooltip = container.querySelector('.' + styles.matchesTooltip);
+    expect(screen.getByText('0 matches')).toBeDefined();
+    userEvent.hover(tooltip!);
+    jest.advanceTimersByTime(1000);
+    await waitFor(() => {
+      expect(screen.getByText(/0 span matches for the filters selected/)).toBeDefined();
+    });
   });
 
   it('renders show span filter matches only switch', async () => {
-    render(<NewTracePageSearchBar {...(defaultProps as unknown as TracePageSearchBarProps)} />);
+    render(<NewTracePageSearchBarWithProps matches={[]} />);
     const matchesSwitch = screen.getByRole('checkbox', { name: 'Show matches only switch' });
     expect(matchesSwitch).toBeInTheDocument();
   });

public/app/features/explore/TraceView/components/TracePageHeader/NewTracePageSearchBar.tsx

@@ -13,42 +13,50 @@
 // limitations under the License.
 
 import { css } from '@emotion/css';
-import React, { memo, Dispatch, SetStateAction, useEffect, useMemo } from 'react';
+import React, { memo, Dispatch, SetStateAction, useEffect, useMemo, useState } from 'react';
 
 import { config, reportInteraction } from '@grafana/runtime';
-import { Button, Switch, useStyles2 } from '@grafana/ui';
+import { Button, Icon, Switch, Tooltip, useStyles2 } from '@grafana/ui';
 
 import { SearchProps } from '../../useSearch';
 import { convertTimeFilter } from '../utils/filter-spans';
 
 export type TracePageSearchBarProps = {
   search: SearchProps;
-  setSearch: React.Dispatch<React.SetStateAction<SearchProps>>;
   spanFilterMatches: Set<string> | undefined;
   showSpanFilterMatchesOnly: boolean;
   setShowSpanFilterMatchesOnly: (showMatchesOnly: boolean) => void;
-  focusedSpanIdForSearch: string;
   setFocusedSpanIdForSearch: Dispatch<SetStateAction<string>>;
   datasourceType: string;
   reset: () => void;
+  totalSpans: number;
 };
 
 export default memo(function NewTracePageSearchBar(props: TracePageSearchBarProps) {
   const {
     search,
     spanFilterMatches,
-    focusedSpanIdForSearch,
+    showSpanFilterMatchesOnly,
+    setShowSpanFilterMatchesOnly,
     setFocusedSpanIdForSearch,
     datasourceType,
     reset,
-    showSpanFilterMatchesOnly,
-    setShowSpanFilterMatchesOnly,
+    totalSpans,
   } = props;
+  const [currentSpanIndex, setCurrentSpanIndex] = useState(-1);
   const styles = useStyles2(getStyles);
 
   useEffect(() => {
+    setCurrentSpanIndex(-1);
     setFocusedSpanIdForSearch('');
-  }, [search, setFocusedSpanIdForSearch]);
+  }, [setFocusedSpanIdForSearch, spanFilterMatches]);
+
+  useEffect(() => {
+    if (spanFilterMatches) {
+      const spanMatches = Array.from(spanFilterMatches!);
+      setFocusedSpanIdForSearch(spanMatches[currentSpanIndex]);
+    }
+  }, [currentSpanIndex, setFocusedSpanIdForSearch, spanFilterMatches]);
 
   const nextResult = () => {
     reportInteraction('grafana_traces_trace_view_find_next_prev_clicked', {
@@ -57,17 +65,14 @@ export default memo(function NewTracePageSearchBar(props: TracePageSearchBarProp
       direction: 'next',
     });
 
-    const spanMatches = Array.from(spanFilterMatches!);
-    const prevMatchedIndex = spanMatches.indexOf(focusedSpanIdForSearch);
-
     // new query || at end, go to start
-    if (prevMatchedIndex === -1 || prevMatchedIndex === spanMatches.length - 1) {
-      setFocusedSpanIdForSearch(spanMatches[0]);
+    if (currentSpanIndex === -1 || (spanFilterMatches && currentSpanIndex === spanFilterMatches.size - 1)) {
+      setCurrentSpanIndex(0);
       return;
     }
 
     // get next
-    setFocusedSpanIdForSearch(spanMatches[prevMatchedIndex + 1]);
+    setCurrentSpanIndex(currentSpanIndex + 1);
   };
 
   const prevResult = () => {
@@ -77,19 +82,17 @@ export default memo(function NewTracePageSearchBar(props: TracePageSearchBarProp
       direction: 'prev',
     });
 
-    const spanMatches = Array.from(spanFilterMatches!);
-    const prevMatchedIndex = spanMatches.indexOf(focusedSpanIdForSearch);
-
     // new query || at start, go to end
-    if (prevMatchedIndex === -1 || prevMatchedIndex === 0) {
-      setFocusedSpanIdForSearch(spanMatches[spanMatches.length - 1]);
+    if (spanFilterMatches && (currentSpanIndex === -1 || currentSpanIndex === 0)) {
+      setCurrentSpanIndex(spanFilterMatches.size - 1);
       return;
     }
 
     // get prev
-    setFocusedSpanIdForSearch(spanMatches[prevMatchedIndex - 1]);
+    setCurrentSpanIndex(currentSpanIndex - 1);
   };
 
+  const buttonEnabled = spanFilterMatches && spanFilterMatches?.size > 0;
   const resetEnabled = useMemo(() => {
     return (
       (search.serviceName && search.serviceName !== '') ||
@@ -102,7 +105,26 @@ export default memo(function NewTracePageSearchBar(props: TracePageSearchBarProp
       })
     );
   }, [search.serviceName, search.spanName, search.from, search.to, search.tags]);
-  const buttonEnabled = spanFilterMatches && spanFilterMatches?.size > 0;
+
+  const amountText = spanFilterMatches?.size === 1 ? 'match' : 'matches';
+  const matches =
+    spanFilterMatches?.size === 0 ? (
+      <>
+        <span>0 matches</span>
+        <Tooltip
+          content="There are 0 span matches for the filters selected. Please try removing some of the selected filters."
+          placement="left"
+        >
+          <span className={styles.matchesTooltip}>
+            <Icon name="info-circle" size="lg" />
+          </span>
+        </Tooltip>
+      </>
+    ) : currentSpanIndex !== -1 ? (
+      `${currentSpanIndex + 1}/${spanFilterMatches?.size} ${amountText}`
+    ) : (
+      `${spanFilterMatches?.size} ${amountText}`
+    );
 
   return (
     <div className={styles.searchBar}>
@@ -129,6 +151,7 @@ export default memo(function NewTracePageSearchBar(props: TracePageSearchBarProp
             </div>
           </div>
           <div className={styles.nextPrevButtons}>
+            <span className={styles.matches}>{spanFilterMatches ? matches : `${totalSpans} spans`}</span>
             <Button
               variant="secondary"
               disabled={!buttonEnabled}
@@ -187,5 +210,12 @@ export const getStyles = () => {
         margin-left: 8px;
       }
     `,
+    matches: css`
+      margin-right: 5px;
+    `,
+    matchesTooltip: css`
+      color: #aaa;
+      margin: -2px 0 0 10px;
+    `,
   };
 };

public/app/features/explore/TraceView/components/TracePageHeader/SpanFilters/SpanFilters.test.tsx

@@ -45,10 +45,9 @@ describe('SpanFilters', () => {
       setShowSpanFilters: jest.fn(),
       showSpanFilterMatchesOnly: false,
       setShowSpanFilterMatchesOnly: jest.fn(),
-      search: search,
-      setSearch: setSearch,
+      search,
+      setSearch,
       spanFilterMatches: undefined,
-      focusedSpanIdForSearch: '',
       setFocusedSpanIdForSearch: jest.fn(),
       datasourceType: 'tempo',
     };

public/app/features/explore/TraceView/components/TracePageHeader/SpanFilters/SpanFilters.tsx

@@ -42,7 +42,6 @@ export type SpanFilterProps = {
   setShowSpanFilters: (isOpen: boolean) => void;
   showSpanFilterMatchesOnly: boolean;
   setShowSpanFilterMatchesOnly: (showMatchesOnly: boolean) => void;
-  focusedSpanIdForSearch: string;
   setFocusedSpanIdForSearch: React.Dispatch<React.SetStateAction<string>>;
   spanFilterMatches: Set<string> | undefined;
   datasourceType: string;
@@ -57,7 +56,6 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
     setShowSpanFilters,
     showSpanFilterMatchesOnly,
     setShowSpanFilterMatchesOnly,
-    focusedSpanIdForSearch,
     setFocusedSpanIdForSearch,
     spanFilterMatches,
     datasourceType,
@@ -89,13 +87,7 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
       const serviceNames = trace.spans.map((span) => {
         return span.process.serviceName;
       });
-      setServiceNames(
-        uniq(serviceNames)
-          .sort()
-          .map((name) => {
-            return toOption(name);
-          })
-      );
+      setServiceNames(uniq(serviceNames).sort().map(toOption));
     }
   };
 
@@ -104,13 +96,7 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
       const spanNames = trace.spans.map((span) => {
         return span.operationName;
       });
-      setSpanNames(
-        uniq(spanNames)
-          .sort()
-          .map((name) => {
-            return toOption(name);
-          })
-      );
+      setSpanNames(uniq(spanNames).sort().map(toOption));
     }
   };
 
@@ -137,11 +123,7 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
       keys = uniq(keys).sort();
       logKeys = uniq(logKeys).sort();
 
-      setTagKeys(
-        [...keys, ...logKeys].map((name) => {
-          return toOption(name);
-        })
-      );
+      setTagKeys([...keys, ...logKeys].map(toOption));
     }
   };
 
@@ -167,11 +149,7 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
       }
     });
 
-    return uniq(values)
-      .sort()
-      .map((name) => {
-        return toOption(name);
-      });
+    return uniq(values).sort().map(toOption);
   };
 
   const onTagChange = (tag: Tag, v: SelectableValue<string>) => {
@@ -228,9 +206,9 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
       content="Filter your spans below. The more filters, the more specific the filtered spans."
       placement="right"
     >
-      <span id="collapse-label">
+      <span className={styles.collapseLabel}>
         Span Filters
-        <Icon size="sm" name="info-circle" />
+        <Icon size="md" name="info-circle" />
       </span>
     </Tooltip>
   );
@@ -319,7 +297,7 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
                 <div key={i}>
                   <HorizontalGroup spacing={'xs'} width={'auto'}>
                     <Select
-                      aria-label={`Select tag key`}
+                      aria-label="Select tag key"
                       isClearable
                       key={tag.key}
                       onChange={(v) => onTagChange(tag, v)}
@@ -329,7 +307,7 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
                       value={tag.key || null}
                     />
                     <Select
-                      aria-label={`Select tag operator`}
+                      aria-label="Select tag operator"
                       onChange={(v) => {
                         setSearch({
                           ...search,
@@ -343,7 +321,7 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
                     />
                     <span className={styles.tagValues}>
                       <Select
-                        aria-label={`Select tag value`}
+                        aria-label="Select tag value"
                         isClearable
                         key={tag.value}
                         onChange={(v) => {
@@ -360,20 +338,20 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
                       />
                     </span>
                     <AccessoryButton
-                      aria-label={`Remove tag`}
-                      variant={'secondary'}
-                      icon={'times'}
+                      aria-label="Remove tag"
+                      variant="secondary"
+                      icon="times"
                       onClick={() => removeTag(tag.id)}
-                      title={'Remove tag'}
+                      title="Remove tag"
                     />
                     <span className={styles.addTag}>
                       {search?.tags?.length && i === search.tags.length - 1 && (
                         <AccessoryButton
                           aria-label="Add tag"
-                          variant={'secondary'}
-                          icon={'plus'}
+                          variant="secondary"
+                          icon="plus"
                           onClick={addTag}
-                          title={'Add tag'}
+                          title="Add tag"
                         />
                       )}
                     </span>
@@ -386,14 +364,13 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
 
         <NewTracePageSearchBar
           search={search}
-          setSearch={setSearch}
           spanFilterMatches={spanFilterMatches}
           showSpanFilterMatchesOnly={showSpanFilterMatchesOnly}
           setShowSpanFilterMatchesOnly={setShowSpanFilterMatchesOnly}
-          focusedSpanIdForSearch={focusedSpanIdForSearch}
           setFocusedSpanIdForSearch={setFocusedSpanIdForSearch}
           datasourceType={datasourceType}
           reset={reset}
+          totalSpans={trace.spans.length}
         />
       </Collapse>
     </div>
@@ -412,9 +389,11 @@ const getStyles = () => {
         border-left: none;
         border-right: none;
       }
-
-      #collapse-label svg {
-        margin: -1px 0 0 10px;
+    `,
+    collapseLabel: css`
+      svg {
+        color: #aaa;
+        margin: -2px 0 0 10px;
       }
     `,
     addTag: css`