From e426039d6c97364be4b0ee8dc9aa00b7145e1cb9 Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Tue, 28 Sep 2021 13:20:49 -0400 Subject: [PATCH] Tempo: Improve search form defaults and validation (#39534) (#39749) * Tempo: add default limit, option to hide Loki search, and run query on hotkey in dropdowns (cherry picked from commit 06592410b24fb957860f4d3cb62cf3980914812c) Co-authored-by: Connor Lindsey --- .../src/datetime/durationutil.test.ts | 32 ++++++++++- .../grafana-data/src/datetime/durationutil.ts | 54 +++++++++++++++++- .../core/components/TraceToLogsSettings.tsx | 15 +++++ .../plugins/datasource/tempo/NativeSearch.tsx | 46 ++++++++++++--- .../plugins/datasource/tempo/QueryField.tsx | 2 +- .../datasource/tempo/datasource.test.ts | 15 +++++ .../plugins/datasource/tempo/datasource.ts | 57 +++++++++++++++---- 7 files changed, 199 insertions(+), 22 deletions(-) diff --git a/packages/grafana-data/src/datetime/durationutil.test.ts b/packages/grafana-data/src/datetime/durationutil.test.ts index 7912c653372..e4c86091edd 100644 --- a/packages/grafana-data/src/datetime/durationutil.test.ts +++ b/packages/grafana-data/src/datetime/durationutil.test.ts @@ -1,4 +1,10 @@ -import { intervalToAbbreviatedDurationString, addDurationToDate, parseDuration } from './durationutil'; +import { + intervalToAbbreviatedDurationString, + addDurationToDate, + parseDuration, + isValidDuration, + isValidGoDuration, +} from './durationutil'; describe('Duration util', () => { describe('intervalToAbbreviatedDurationString', () => { @@ -20,4 +26,28 @@ describe('Duration util', () => { expect(parseDuration(durationString)).toEqual({ months: '3', minutes: '4' }); }); }); + + describe('isValidDuration', () => { + it('valid duration string returns true', () => { + const durationString = '3M 5d 20m'; + expect(isValidDuration(durationString)).toEqual(true); + }); + + it('invalid duration string returns false', () => { + const durationString = '3M 6v 5b 4m'; + expect(isValidDuration(durationString)).toEqual(false); + }); + }); + + describe('isValidGoDuration', () => { + it('valid duration string returns true', () => { + const durationString = '3h 4m 1s 2ms 3us 5ns'; + expect(isValidGoDuration(durationString)).toEqual(true); + }); + + it('invalid duration string returns false', () => { + const durationString = '3M 6v 5b 4m'; + expect(isValidGoDuration(durationString)).toEqual(false); + }); + }); }); diff --git a/packages/grafana-data/src/datetime/durationutil.ts b/packages/grafana-data/src/datetime/durationutil.ts index 542e7ea27d2..9b318804936 100644 --- a/packages/grafana-data/src/datetime/durationutil.ts +++ b/packages/grafana-data/src/datetime/durationutil.ts @@ -13,7 +13,7 @@ const durationMap: { [key in Required]: string[] } = { }; /** - * intervalToAbbreviatedDurationString convers interval to readable duration string + * intervalToAbbreviatedDurationString converts interval to readable duration string * * @param interval - interval to convert * @param includeSeconds - optional, default true. If false, will not include seconds unless interval is less than 1 minute @@ -85,3 +85,55 @@ export function durationToMilliseconds(duration: Duration): number { export function isValidDate(dateString: string): boolean { return !isNaN(Date.parse(dateString)); } + +/** + * isValidDuration returns true if the given string can be parsed into a valid Duration object, false otherwise + * + * @param durationString - string representation of a duration + * + * @public + */ +export function isValidDuration(durationString: string): boolean { + for (const value of durationString.trim().split(' ')) { + const match = value.match(/(\d+)(.+)/); + if (match === null || match.length !== 3) { + return false; + } + + const key = Object.entries(durationMap).find(([_, abbreviations]) => abbreviations?.includes(match[2]))?.[0]; + if (!key) { + return false; + } + } + + return true; +} + +/** + * isValidGoDuration returns true if the given string can be parsed into a valid Duration object based on + * Go's time.parseDuration, false otherwise. + * + * Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h". + * + * Go docs: https://pkg.go.dev/time#ParseDuration + * + * @param durationString - string representation of a duration + * + * @internal + */ +export function isValidGoDuration(durationString: string): boolean { + const timeUnits = ['h', 'm', 's', 'ms', 'us', 'µs', 'ns']; + for (const value of durationString.trim().split(' ')) { + const match = value.match(/(\d+)(.+)/); + if (match === null || match.length !== 3) { + return false; + } + + const isValidUnit = timeUnits.includes(match[2]); + if (!isValidUnit) { + return false; + } + } + + return true; +} diff --git a/public/app/core/components/TraceToLogsSettings.tsx b/public/app/core/components/TraceToLogsSettings.tsx index 64ab1668c17..40fc24f73e0 100644 --- a/public/app/core/components/TraceToLogsSettings.tsx +++ b/public/app/core/components/TraceToLogsSettings.tsx @@ -16,6 +16,7 @@ export interface TraceToLogsOptions { spanEndTimeShift?: string; filterByTraceID?: boolean; filterBySpanID?: boolean; + lokiSearch?: boolean; } export interface TraceToLogsData extends DataSourceJsonData { @@ -152,6 +153,20 @@ export function TraceToLogsSettings({ options, onOptionsChange }: Props) { /> + + + ) => + updateDatasourcePluginJsonDataOption({ onOptionsChange, options }, 'tracesToLogs', { + ...options.jsonData.tracesToLogs, + lokiSearch: event.currentTarget.checked, + }) + } + /> + + ); } diff --git a/public/app/plugins/datasource/tempo/NativeSearch.tsx b/public/app/plugins/datasource/tempo/NativeSearch.tsx index 663ccd284ad..0e6743305a9 100644 --- a/public/app/plugins/datasource/tempo/NativeSearch.tsx +++ b/public/app/plugins/datasource/tempo/NativeSearch.tsx @@ -16,7 +16,7 @@ import { tokenizer } from './syntax'; import Prism from 'prismjs'; import { Node } from 'slate'; import { css } from '@emotion/css'; -import { GrafanaTheme2, SelectableValue } from '@grafana/data'; +import { GrafanaTheme2, isValidGoDuration, SelectableValue } from '@grafana/data'; import TempoLanguageProvider from './language_provider'; import { TempoDatasource, TempoQuery } from './datasource'; import { debounce } from 'lodash'; @@ -56,6 +56,7 @@ const NativeSearch = ({ datasource, query, onChange, onBlur, onRunQuery }: Props spanNameOptions: [], }); const [error, setError] = useState(null); + const [inputErrors, setInputErrors] = useState<{ [key: string]: boolean }>({}); const fetchServiceNameOptions = useMemo( () => @@ -139,6 +140,7 @@ const NativeSearch = ({ datasource, query, onChange, onBlur, onRunQuery }: Props placeholder="Select a service" onOpenMenu={fetchServiceNameOptions} isClearable + onKeyDown={onKeyDown} /> @@ -157,6 +159,7 @@ const NativeSearch = ({ datasource, query, onChange, onBlur, onRunQuery }: Props placeholder="Select a span" onOpenMenu={fetchSpanNameOptions} isClearable + onKeyDown={onKeyDown} /> @@ -182,10 +185,17 @@ const NativeSearch = ({ datasource, query, onChange, onBlur, onRunQuery }: Props - + { + if (query.minDuration && !isValidGoDuration(query.minDuration)) { + setInputErrors({ ...inputErrors, minDuration: true }); + } else { + setInputErrors({ ...inputErrors, minDuration: false }); + } + }} onChange={(v) => onChange({ ...query, @@ -197,10 +207,17 @@ const NativeSearch = ({ datasource, query, onChange, onBlur, onRunQuery }: Props - + { + if (query.maxDuration && !isValidGoDuration(query.maxDuration)) { + setInputErrors({ ...inputErrors, maxDuration: true }); + } else { + setInputErrors({ ...inputErrors, maxDuration: false }); + } + }} onChange={(v) => onChange({ ...query, @@ -212,16 +229,29 @@ const NativeSearch = ({ datasource, query, onChange, onBlur, onRunQuery }: Props - + + onChange={(v) => { + let limit = v.currentTarget.value ? parseInt(v.currentTarget.value, 10) : undefined; + if (limit && (!Number.isInteger(limit) || limit <= 0)) { + setInputErrors({ ...inputErrors, limit: true }); + } else { + setInputErrors({ ...inputErrors, limit: false }); + } + onChange({ ...query, limit: v.currentTarget.value ? parseInt(v.currentTarget.value, 10) : undefined, - }) - } + }); + }} onKeyDown={onKeyDown} /> @@ -230,7 +260,7 @@ const NativeSearch = ({ datasource, query, onChange, onBlur, onRunQuery }: Props {error ? ( Please ensure that Tempo is configured with search enabled. If you would like to hide this tab, you can - configure it in the datasource settings. + configure it in the datasource settings. ) : null} diff --git a/public/app/plugins/datasource/tempo/QueryField.tsx b/public/app/plugins/datasource/tempo/QueryField.tsx index 29ab4cf2a28..6cd3822b1ba 100644 --- a/public/app/plugins/datasource/tempo/QueryField.tsx +++ b/public/app/plugins/datasource/tempo/QueryField.tsx @@ -103,7 +103,7 @@ class TempoQueryFieldComponent extends React.PureComponent { queryTypeOptions.unshift({ value: 'nativeSearch', label: 'Search - Beta' }); } - if (logsDatasourceUid) { + if (logsDatasourceUid && tracesToLogsOptions?.lokiSearch !== false) { if (!config.featureToggles.tempoSearch) { // Place at beginning as Search if no native search queryTypeOptions.unshift({ value: 'search', label: 'Search' }); diff --git a/public/app/plugins/datasource/tempo/datasource.test.ts b/public/app/plugins/datasource/tempo/datasource.test.ts index 58f9e3be4f9..0c465841a2a 100644 --- a/public/app/plugins/datasource/tempo/datasource.test.ts +++ b/public/app/plugins/datasource/tempo/datasource.test.ts @@ -155,6 +155,20 @@ describe('Tempo data source', () => { }); }); + it('should include a default limit of 100', () => { + const ds = new TempoDatasource(defaultSettings); + const tempoQuery: TempoQuery = { + queryType: 'search', + refId: 'A', + query: '', + search: '', + }; + const builtQuery = ds.buildSearchQuery(tempoQuery); + expect(builtQuery).toStrictEqual({ + limit: 100, + }); + }); + it('should ignore incomplete tag queries', () => { const ds = new TempoDatasource(defaultSettings); const tempoQuery: TempoQuery = { @@ -165,6 +179,7 @@ describe('Tempo data source', () => { }; const builtQuery = ds.buildSearchQuery(tempoQuery); expect(builtQuery).toStrictEqual({ + limit: 100, 'root.http.status_code': '500', }); }); diff --git a/public/app/plugins/datasource/tempo/datasource.ts b/public/app/plugins/datasource/tempo/datasource.ts index 486bb7e8b2b..cebcd5f928d 100644 --- a/public/app/plugins/datasource/tempo/datasource.ts +++ b/public/app/plugins/datasource/tempo/datasource.ts @@ -1,5 +1,5 @@ import { from, merge, Observable, of, throwError } from 'rxjs'; -import { map, mergeMap, toArray } from 'rxjs/operators'; +import { catchError, map, mergeMap, toArray } from 'rxjs/operators'; import { DataQuery, DataQueryRequest, @@ -7,6 +7,7 @@ import { DataSourceApi, DataSourceInstanceSettings, DataSourceJsonData, + isValidGoDuration, LoadingState, } from '@grafana/data'; import { TraceToLogsOptions } from 'app/core/components/TraceToLogsSettings'; @@ -109,16 +110,23 @@ export class TempoDatasource extends DataSourceWithBackend { - return { - data: [createTableFrameFromSearch(response.data.traces, this.instanceSettings)], - }; - }) - ) - ); + try { + const searchQuery = this.buildSearchQuery(targets.nativeSearch[0]); + subQueries.push( + this._request('/api/search', searchQuery).pipe( + map((response) => { + return { + data: [createTableFrameFromSearch(response.data.traces, this.instanceSettings)], + }; + }), + catchError((error) => { + return of({ error: { message: error.data.message }, data: [] }); + }) + ) + ); + } catch (error) { + return of({ error: { message: error.message }, data: [] }); + } } if (targets.upload?.length) { @@ -204,6 +212,8 @@ export class TempoDatasource extends DataSourceWithBackend ({ ...tagQuery, ...item }), {}); return { ...tagsQueryObject, ...tempoQuery }; }