From ea96c83a2c27c7e0501274507febfc4ef1447c19 Mon Sep 17 00:00:00 2001 From: Joey <90795735+joey-grafana@users.noreply.github.com> Date: Thu, 8 Feb 2024 09:24:23 +0000 Subject: [PATCH] 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 | 3 +- .../SpanFilters/SpanFilters.test.tsx | 38 ++++++++++-- .../SpanFilters/SpanFilters.tsx | 32 +++++----- .../SearchTraceQLEditor/GroupByField.test.tsx | 48 ++++++++++++++- .../SearchTraceQLEditor/GroupByField.tsx | 26 ++++---- .../tempo/SearchTraceQLEditor/SearchField.tsx | 14 ----- .../tempo/SearchTraceQLEditor/TagsInput.tsx | 40 +++++++++--- .../TraceQLSearch.test.tsx | 61 ++++++++++++++++++- .../SearchTraceQLEditor/TraceQLSearch.tsx | 1 + 9 files changed, 205 insertions(+), 58 deletions(-) diff --git a/.betterer.results b/.betterer.results index 23c6330def8..1d488edfe10 100644 --- a/.betterer.results +++ b/.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.", "6"] + [0, 0, 0, "Styles should be written using objects.", "5"] ], "public/app/features/explore/TraceView/components/TracePageHeader/SpanGraph/CanvasSpanGraph.tsx:5381": [ [0, 0, 0, "Styles should be written using objects.", "0"] diff --git a/public/app/features/explore/TraceView/components/TracePageHeader/SpanFilters/SpanFilters.test.tsx b/public/app/features/explore/TraceView/components/TracePageHeader/SpanFilters/SpanFilters.test.tsx index dd7318d880c..0f4f697978d 100644 --- a/public/app/features/explore/TraceView/components/TracePageHeader/SpanFilters/SpanFilters.test.tsx +++ b/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(); + 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(); 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(); }); diff --git a/public/app/features/explore/TraceView/components/TracePageHeader/SpanFilters/SpanFilters.tsx b/public/app/features/explore/TraceView/components/TracePageHeader/SpanFilters/SpanFilters.tsx index 5878c76d2a8..6de82f34043 100644 --- a/public/app/features/explore/TraceView/components/TracePageHeader/SpanFilters/SpanFilters.tsx +++ b/public/app/features/explore/TraceView/components/TracePageHeader/SpanFilters/SpanFilters.tsx @@ -439,24 +439,26 @@ export const SpanFilters = memo((props: SpanFilterProps) => { value={tag.value} /> - removeTag(tag.id)} - title="Remove tag" - /> - - {search?.tags?.length && i === search.tags.length - 1 && ( + {(tag.key || tag.value || search.tags.length > 1) && ( + removeTag(tag.id)} + tooltip="Remove tag" + /> + )} + {(tag.key || tag.value) && i === search.tags.length - 1 && ( + - )} - + + )} ))} @@ -508,9 +510,9 @@ const getStyles = (theme: GrafanaTheme2) => { display: 'flex', justifyContent: 'space-between', }), - addTag: css` - margin: 0 0 0 10px; - `, + addTag: css({ + marginLeft: theme.spacing(1), + }), intervalInput: css` margin: 0 -4px 0 0; `, diff --git a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/GroupByField.test.tsx b/public/app/plugins/datasource/tempo/SearchTraceQLEditor/GroupByField.test.tsx index af230d44ecf..95ebbb01afc 100644 --- a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/GroupByField.test.tsx +++ b/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({ + refId: 'A', + queryType: 'traceqlSearch', + key: 'Q-595a9bbc-2a25-49a7-9249-a52a0a475d83-0', + filters: [], + groupBy: [{ id: 'group-by-id', scope: TraceqlSearchScope.Span }], + }); + return ( + setQuery(q)} + isTagsLoading={false} + /> + ); + }; + render(); + 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( diff --git a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/GroupByField.tsx b/public/app/plugins/datasource/tempo/SearchTraceQLEditor/GroupByField.tsx index 27b7cb86590..e88ab32a21c 100644 --- a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/GroupByField.tsx +++ b/public/app/plugins/datasource/tempo/SearchTraceQLEditor/GroupByField.tsx @@ -101,16 +101,18 @@ export const GroupByField = (props: Props) => { placeholder="Select tag" value={f.tag || ''} /> - removeFilter(f)} - tooltip="Remove tag" - variant="secondary" - /> - - {i === (query.groupBy?.length ?? 0) - 1 && ( - + {(f.tag || (query.groupBy?.length ?? 0) > 1) && ( + removeFilter(f)} + tooltip="Remove tag" + title={`Remove tag for filter ${i + 1}`} + variant="secondary" + /> + )} + {f.tag && i === (query.groupBy?.length ?? 0) - 1 && ( + { }; const getStyles = (theme: GrafanaTheme2) => ({ - addFilter: css({ - marginLeft: theme.spacing(2), + addTag: css({ + marginLeft: theme.spacing(1), }), }); diff --git a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/SearchField.tsx b/public/app/plugins/datasource/tempo/SearchTraceQLEditor/SearchField.tsx index c16095acf0d..e0a8519dd5a 100644 --- a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/SearchField.tsx +++ b/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 && ( - deleteFilter?.(filter)} - tooltip={'Remove tag'} - aria-label={`remove tag with ID ${filter.id}`} - /> - )} ); }; diff --git a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/TagsInput.tsx b/public/app/plugins/datasource/tempo/SearchTraceQLEditor/TagsInput.tsx index 63a703ac8b4..eed0e73384f 100644 --- a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/TagsInput.tsx +++ b/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; 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 (
{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) && ( + deleteFilter?.(f)} + tooltip={'Remove tag'} + /> + )} + {validInput(f) && i === filters.length - 1 && ( + + + )}
))} diff --git a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/TraceQLSearch.test.tsx b/public/app/plugins/datasource/tempo/SearchTraceQLEditor/TraceQLSearch.test.tsx index 6b13f2bc871..1240317deaa 100644 --- a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/TraceQLSearch.test.tsx +++ b/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 { act } from 'react-dom/test-utils'; +import React, { useState } from 'react'; 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({ + refId: 'A', + queryType: 'traceqlSearch', + key: 'Q-595a9bbc-2a25-49a7-9249-a52a0a475d83-0', + filters: [], + }); + return ( + setQuery(q)} + onClearResults={onClearResults} + /> + ); + }; + render(); + + 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( diff --git a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/TraceQLSearch.tsx b/public/app/plugins/datasource/tempo/SearchTraceQLEditor/TraceQLSearch.tsx index 852c290ba46..0a03044c63a 100644 --- a/public/app/plugins/datasource/tempo/SearchTraceQLEditor/TraceQLSearch.tsx +++ b/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} /> {config.featureToggles.metricsSummary && (