Tempo: Improve tags UX (#81166)

* Only show add/remove tag when necessary in span filters

* Only show add/remove tag when necessary in traceql

* Only show add/remove tag when necessary in aggregateBy

* Update styles

* Add tests

* Show remove tag for all if more than one tag

* Also check for value only in search editor and update tests

.betterer.results

@@ -3421,8 +3421,7 @@ exports[`better eslint`] = {
       [0, 0, 0, "Styles should be written using objects.", "2"],
       [0, 0, 0, "Styles should be written using objects.", "3"],
       [0, 0, 0, "Styles should be written using objects.", "4"],
-      [0, 0, 0, "Styles should be written using objects.", "5"],
+      [0, 0, 0, "Styles should be written using objects.", "5"]
-      [0, 0, 0, "Styles should be written using objects.", "6"]
     ],
     "public/app/features/explore/TraceView/components/TracePageHeader/SpanGraph/CanvasSpanGraph.tsx:5381": [
       [0, 0, 0, "Styles should be written using objects.", "0"]

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

@@ -100,8 +100,6 @@ describe('SpanFilters', () => {
     const tagKey = screen.getByLabelText('Select tag key');
     const tagOperator = screen.getByLabelText('Select tag operator');
     const tagValue = screen.getByLabelText('Select tag value');
-    const addTag = screen.getByLabelText('Add tag');
-    const removeTag = screen.getByLabelText('Remove tag');
 
     expect(serviceOperator).toBeInTheDocument();
     expect(getElemText(serviceOperator)).toBe('=');
@@ -119,8 +117,6 @@ describe('SpanFilters', () => {
     expect(tagOperator).toBeInTheDocument();
     expect(getElemText(tagOperator)).toBe('=');
     expect(tagValue).toBeInTheDocument();
-    expect(addTag).toBeInTheDocument();
-    expect(removeTag).toBeInTheDocument();
 
     await user.click(serviceValue);
     jest.advanceTimersByTime(1000);
@@ -194,9 +190,41 @@ describe('SpanFilters', () => {
     });
   });
 
+  it('should only show add/remove tag when necessary', async () => {
+    render(<SpanFiltersWithProps />);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(0); // not filled in the default tag, so no need to add another one
+    expect(screen.queryAllByLabelText('Remove tag').length).toBe(0); // mot filled in the default tag, so no values to remove
+    expect(screen.getAllByLabelText('Select tag key').length).toBe(1);
+
+    await selectAndCheckValue(user, screen.getByLabelText('Select tag key'), 'TagKey0');
+    expect(screen.getAllByLabelText('Add tag').length).toBe(1);
+    expect(screen.getAllByLabelText('Remove tag').length).toBe(1);
+
+    await user.click(screen.getByLabelText('Add tag'));
+    jest.advanceTimersByTime(1000);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(0); // not filled in the new tag, so no need to add another one
+    expect(screen.getAllByLabelText('Remove tag').length).toBe(2); // one for each tag
+    expect(screen.getAllByLabelText('Select tag key').length).toBe(2);
+
+    await user.click(screen.getAllByLabelText('Remove tag')[1]);
+    jest.advanceTimersByTime(1000);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(1); // filled in the default tag, so can add another one
+    expect(screen.queryAllByLabelText('Remove tag').length).toBe(1); // filled in the default tag, so can remove values
+    expect(screen.getAllByLabelText('Select tag key').length).toBe(1);
+
+    await user.click(screen.getAllByLabelText('Remove tag')[0]);
+    jest.advanceTimersByTime(1000);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(0); // not filled in the default tag, so no need to add another one
+    expect(screen.queryAllByLabelText('Remove tag').length).toBe(0); // mot filled in the default tag, so no values to remove
+    expect(screen.getAllByLabelText('Select tag key').length).toBe(1);
+  });
+
   it('should allow adding/removing tags', async () => {
     render(<SpanFiltersWithProps />);
     expect(screen.getAllByLabelText('Select tag key').length).toBe(1);
+    const tagKey = screen.getByLabelText('Select tag key');
+    await selectAndCheckValue(user, tagKey, 'TagKey0');
+
     await user.click(screen.getByLabelText('Add tag'));
     jest.advanceTimersByTime(1000);
     expect(screen.getAllByLabelText('Select tag key').length).toBe(2);
@@ -232,6 +260,8 @@ describe('SpanFilters', () => {
     expect(screen.queryByText('Span0')).not.toBeInTheDocument();
     expect(screen.queryByText('TagKey0')).not.toBeInTheDocument();
     expect(screen.queryByText('TagValue0')).not.toBeInTheDocument();
+    expect(screen.queryByText('Add tag')).not.toBeInTheDocument();
+    expect(screen.queryByText('Remove tag')).not.toBeInTheDocument();
     expect(matchesSwitch).not.toBeChecked();
   });
 

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

@@ -439,24 +439,26 @@ export const SpanFilters = memo((props: SpanFilterProps) => {
                         value={tag.value}
                       />
                     </span>
-                    <AccessoryButton
+                    {(tag.key || tag.value || search.tags.length > 1) && (
-                      aria-label="Remove tag"
+                      <AccessoryButton
-                      variant="secondary"
+                        aria-label="Remove tag"
-                      icon="times"
+                        variant="secondary"
-                      onClick={() => removeTag(tag.id)}
+                        icon="times"
-                      title="Remove tag"
+                        onClick={() => removeTag(tag.id)}
-                    />
+                        tooltip="Remove tag"
-                    <span className={styles.addTag}>
+                      />
-                      {search?.tags?.length && i === search.tags.length - 1 && (
+                    )}
+                    {(tag.key || tag.value) && i === search.tags.length - 1 && (
+                      <span className={styles.addTag}>
                         <AccessoryButton
                           aria-label="Add tag"
                           variant="secondary"
                           icon="plus"
                           onClick={addTag}
-                          title="Add tag"
+                          tooltip="Add tag"
                         />
-                      )}
+                      </span>
-                    </span>
+                    )}
                   </HorizontalGroup>
                 </div>
               ))}
@@ -508,9 +510,9 @@ const getStyles = (theme: GrafanaTheme2) => {
       display: 'flex',
       justifyContent: 'space-between',
     }),
-    addTag: css`
+    addTag: css({
-      margin: 0 0 0 10px;
+      marginLeft: theme.spacing(1),
-    `,
+    }),
     intervalInput: css`
       margin: 0 -4px 0 0;
     `,

public/app/plugins/datasource/tempo/SearchTraceQLEditor/GroupByField.test.tsx

@@ -1,6 +1,6 @@
 import { render, screen } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
-import React from 'react';
+import React, { useState } from 'react';
 
 import { TraceqlSearchScope } from '../dataquery.gen';
 import { TempoDatasource } from '../datasource';
@@ -46,6 +46,52 @@ describe('GroupByField', () => {
     jest.useRealTimers();
   });
 
+  it('should only show add/remove tag when necessary', async () => {
+    const GroupByWithProps = () => {
+      const [query, setQuery] = useState<TempoQuery>({
+        refId: 'A',
+        queryType: 'traceqlSearch',
+        key: 'Q-595a9bbc-2a25-49a7-9249-a52a0a475d83-0',
+        filters: [],
+        groupBy: [{ id: 'group-by-id', scope: TraceqlSearchScope.Span }],
+      });
+      return (
+        <GroupByField
+          datasource={datasource}
+          query={query}
+          onChange={(q: TempoQuery) => setQuery(q)}
+          isTagsLoading={false}
+        />
+      );
+    };
+    render(<GroupByWithProps />);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(0); // not filled in the default tag, so no need to add another one
+    expect(screen.queryAllByLabelText(/Remove tag/).length).toBe(0); // mot filled in the default tag, so no values to remove
+    expect(screen.getAllByText('Select tag').length).toBe(1);
+
+    await user.click(screen.getByText('Select tag'));
+    jest.advanceTimersByTime(1000);
+    await user.click(screen.getByText('http.method'));
+    jest.advanceTimersByTime(1000);
+    expect(screen.getAllByLabelText('Add tag').length).toBe(1);
+    expect(screen.getAllByLabelText(/Remove tag/).length).toBe(1);
+
+    await user.click(screen.getByLabelText('Add tag'));
+    jest.advanceTimersByTime(1000);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(0); // not filled in the new tag, so no need to add another one
+    expect(screen.getAllByLabelText(/Remove tag/).length).toBe(2); // one for each tag
+
+    await user.click(screen.getAllByLabelText(/Remove tag/)[1]);
+    jest.advanceTimersByTime(1000);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(1); // filled in the default tag, so can add another one
+    expect(screen.queryAllByLabelText(/Remove tag/).length).toBe(1); // filled in the default tag, so can remove values
+
+    await user.click(screen.getAllByLabelText(/Remove tag/)[0]);
+    jest.advanceTimersByTime(1000);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(0); // not filled in the default tag, so no need to add another one
+    expect(screen.queryAllByLabelText(/Remove tag/).length).toBe(0); // mot filled in the default tag, so no values to remove
+  });
+
   it('should update scope when new value is selected in scope input', async () => {
     const { container } = render(
       <GroupByField datasource={datasource} query={query} onChange={onChange} isTagsLoading={false} />

public/app/plugins/datasource/tempo/SearchTraceQLEditor/GroupByField.tsx

@@ -101,16 +101,18 @@ export const GroupByField = (props: Props) => {
                 placeholder="Select tag"
                 value={f.tag || ''}
               />
-              <AccessoryButton
+              {(f.tag || (query.groupBy?.length ?? 0) > 1) && (
-                aria-label={`Remove tag for filter ${i + 1}`}
+                <AccessoryButton
-                icon="times"
+                  aria-label={`Remove tag for filter ${i + 1}`}
-                onClick={() => removeFilter(f)}
+                  icon="times"
-                tooltip="Remove tag"
+                  onClick={() => removeFilter(f)}
-                variant="secondary"
+                  tooltip="Remove tag"
-              />
+                  title={`Remove tag for filter ${i + 1}`}
-
+                  variant="secondary"
-              {i === (query.groupBy?.length ?? 0) - 1 && (
+                />
-                <span className={styles.addFilter}>
+              )}
+              {f.tag && i === (query.groupBy?.length ?? 0) - 1 && (
+                <span className={styles.addTag}>
                   <AccessoryButton
                     aria-label="Add tag"
                     icon="plus"
@@ -129,7 +131,7 @@ export const GroupByField = (props: Props) => {
 };
 
 const getStyles = (theme: GrafanaTheme2) => ({
-  addFilter: css({
+  addTag: css({
-    marginLeft: theme.spacing(2),
+    marginLeft: theme.spacing(1),
   }),
 });

public/app/plugins/datasource/tempo/SearchTraceQLEditor/SearchField.tsx

@@ -4,7 +4,6 @@ import React, { useState, useEffect, useMemo } from 'react';
 import useAsync from 'react-use/lib/useAsync';
 
 import { SelectableValue } from '@grafana/data';
-import { AccessoryButton } from '@grafana/experimental';
 import { FetchError, getTemplateSrv, isFetchError } from '@grafana/runtime';
 import { Select, HorizontalGroup, useStyles2 } from '@grafana/ui';
 
@@ -27,28 +26,24 @@ interface Props {
   filter: TraceqlFilter;
   datasource: TempoDatasource;
   updateFilter: (f: TraceqlFilter) => void;
-  deleteFilter?: (f: TraceqlFilter) => void;
   setError: (error: FetchError) => void;
   isTagsLoading?: boolean;
   tags: string[];
   hideScope?: boolean;
   hideTag?: boolean;
   hideValue?: boolean;
-  allowDelete?: boolean;
   query: string;
 }
 const SearchField = ({
   filter,
   datasource,
   updateFilter,
-  deleteFilter,
   isTagsLoading,
   tags,
   setError,
   hideScope,
   hideTag,
   hideValue,
-  allowDelete,
   query,
 }: Props) => {
   const styles = useStyles2(getStyles);
@@ -207,15 +202,6 @@ const SearchField = ({
           allowCreateWhileLoading
         />
       )}
-      {allowDelete && (
-        <AccessoryButton
-          variant={'secondary'}
-          icon={'times'}
-          onClick={() => deleteFilter?.(filter)}
-          tooltip={'Remove tag'}
-          aria-label={`remove tag with ID ${filter.id}`}
-        />
-      )}
     </HorizontalGroup>
   );
 };

public/app/plugins/datasource/tempo/SearchTraceQLEditor/TagsInput.tsx

@@ -2,6 +2,7 @@ import { css } from '@emotion/css';
 import React, { useCallback, useEffect } from 'react';
 import { v4 as uuidv4 } from 'uuid';
 
+import { GrafanaTheme2 } from '@grafana/data';
 import { AccessoryButton } from '@grafana/experimental';
 import { FetchError } from '@grafana/runtime';
 import { useStyles2 } from '@grafana/ui';
@@ -12,16 +13,19 @@ import { TempoDatasource } from '../datasource';
 import SearchField from './SearchField';
 import { getFilteredTags } from './utils';
 
-const getStyles = () => ({
+const getStyles = (theme: GrafanaTheme2) => ({
   vertical: css({
     display: 'flex',
     flexDirection: 'column',
-    gap: '0.25rem',
+    gap: theme.spacing(0.25),
   }),
   horizontal: css({
     display: 'flex',
     flexDirection: 'row',
-    gap: '1rem',
+    gap: theme.spacing(1),
+  }),
+  addTag: css({
+    marginLeft: theme.spacing(1),
   }),
 });
 
@@ -34,6 +38,7 @@ interface Props {
   staticTags: Array<string | undefined>;
   isTagsLoading: boolean;
   hideValues?: boolean;
+  requireTagAndValue?: boolean;
   query: string;
 }
 const TagsInput = ({
@@ -45,6 +50,7 @@ const TagsInput = ({
   staticTags,
   isTagsLoading,
   hideValues,
+  requireTagAndValue,
   query,
 }: Props) => {
   const styles = useStyles2(getStyles);
@@ -65,6 +71,11 @@ const TagsInput = ({
     return getFilteredTags(tags, staticTags);
   };
 
+  const validInput = (f: TraceqlFilter) => {
+    // If value is removed from the filter, it can be set as an empty array
+    return requireTagAndValue ? f.tag && f.value && f.value.length > 0 : f.tag;
+  };
+
   return (
     <div className={styles.vertical}>
       {filters?.map((f, i) => (
@@ -76,13 +87,28 @@ const TagsInput = ({
             updateFilter={updateFilter}
             tags={getTags(f)}
             isTagsLoading={isTagsLoading}
-            deleteFilter={deleteFilter}
-            allowDelete={true}
             hideValue={hideValues}
             query={query}
           />
-          {i === filters.length - 1 && (
+          {(validInput(f) || filters.length > 1) && (
-            <AccessoryButton variant={'secondary'} icon={'plus'} onClick={handleOnAdd} title={'Add tag'} />
+            <AccessoryButton
+              aria-label={`Remove tag with ID ${f.id}`}
+              variant={'secondary'}
+              icon={'times'}
+              onClick={() => deleteFilter?.(f)}
+              tooltip={'Remove tag'}
+            />
+          )}
+          {validInput(f) && i === filters.length - 1 && (
+            <span className={styles.addTag}>
+              <AccessoryButton
+                aria-label="Add tag"
+                variant={'secondary'}
+                icon={'plus'}
+                onClick={handleOnAdd}
+                tooltip={'Add tag'}
+              />
+            </span>
           )}
         </div>
       ))}

public/app/plugins/datasource/tempo/SearchTraceQLEditor/TraceQLSearch.test.tsx

@@ -1,7 +1,6 @@
-import { render, screen, waitFor } from '@testing-library/react';
+import { act, render, screen, waitFor } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
-import React from 'react';
+import React, { useState } from 'react';
-import { act } from 'react-dom/test-utils';
 
 import { config } from '@grafana/runtime';
 
@@ -89,6 +88,62 @@ describe('TraceQLSearch', () => {
     jest.useRealTimers();
   });
 
+  it('should only show add/remove tag when necessary', async () => {
+    const TraceQLSearchWithProps = () => {
+      const [query, setQuery] = useState<TempoQuery>({
+        refId: 'A',
+        queryType: 'traceqlSearch',
+        key: 'Q-595a9bbc-2a25-49a7-9249-a52a0a475d83-0',
+        filters: [],
+      });
+      return (
+        <TraceQLSearch
+          datasource={datasource}
+          query={query}
+          onChange={(q: TempoQuery) => setQuery(q)}
+          onClearResults={onClearResults}
+        />
+      );
+    };
+    render(<TraceQLSearchWithProps />);
+
+    await act(async () => {
+      expect(screen.queryAllByLabelText('Add tag').length).toBe(0); // not filled in the default tag, so no need to add another one
+      expect(screen.queryAllByLabelText('Remove tag').length).toBe(0); // mot filled in the default tag, so no values to remove
+      expect(screen.getAllByText('Select tag').length).toBe(1);
+    });
+
+    await user.click(screen.getByText('Select tag'));
+    jest.advanceTimersByTime(1000);
+    await user.click(screen.getByText('foo'));
+    jest.advanceTimersByTime(1000);
+    await user.click(screen.getAllByText('Select value')[2]);
+    jest.advanceTimersByTime(1000);
+    await user.click(screen.getByText('driver'));
+    jest.advanceTimersByTime(1000);
+    await act(async () => {
+      expect(screen.getAllByLabelText('Add tag').length).toBe(1);
+      expect(screen.getAllByLabelText(/Remove tag/).length).toBe(1);
+    });
+
+    await user.click(screen.getByLabelText('Add tag'));
+    jest.advanceTimersByTime(1000);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(0); // not filled in the new tag, so no need to add another one
+    expect(screen.getAllByLabelText(/Remove tag/).length).toBe(2); // one for each tag
+
+    await user.click(screen.getAllByLabelText(/Remove tag/)[1]);
+    jest.advanceTimersByTime(1000);
+    expect(screen.queryAllByLabelText('Add tag').length).toBe(1); // filled in the default tag, so can add another one
+    expect(screen.queryAllByLabelText(/Remove tag/).length).toBe(1); // filled in the default tag, so can remove values
+
+    await user.click(screen.getAllByLabelText(/Remove tag/)[0]);
+    jest.advanceTimersByTime(1000);
+    await act(async () => {
+      expect(screen.queryAllByLabelText('Add tag').length).toBe(0); // not filled in the default tag, so no need to add another one
+      expect(screen.queryAllByLabelText(/Remove tag/).length).toBe(0); // mot filled in the default tag, so no values to remove
+    });
+  });
+
   it('should update operator when new value is selected in operator input', async () => {
     const { container } = render(
       <TraceQLSearch datasource={datasource} query={query} onChange={onChange} onClearResults={onClearResults} />

public/app/plugins/datasource/tempo/SearchTraceQLEditor/TraceQLSearch.tsx

@@ -209,6 +209,7 @@ const TraceQLSearch = ({ datasource, query, onChange, onClearResults, app }: Pro
               staticTags={staticTags}
               isTagsLoading={isTagsLoading}
               query={traceQlQuery}
+              requireTagAndValue={true}
             />
           </InlineSearchField>
           {config.featureToggles.metricsSummary && (