From 04c44f0a11db1b0c13ecd41b6847d1235658037f Mon Sep 17 00:00:00 2001 From: Matias Chomicki Date: Mon, 4 Sep 2023 16:30:17 +0200 Subject: [PATCH] Explore Logs: Update log filtering functions to only have effect in the source query (#73626) * isFilterLabelActive: add row parameter to method signature * onClickFilter: add row parameter to method signature * Explore: implement optional row parameter to modify queries * LogDetails: add integration test with filters and log details row * Change signature from LogRowModel to string * Add refid reference to tooltips --- public/app/features/explore/Explore.tsx | 33 +++++++-------- public/app/features/explore/Logs/Logs.tsx | 6 +-- .../features/explore/Logs/LogsContainer.tsx | 6 +-- .../logs/components/LogDetails.test.tsx | 37 +++++++++++++++++ .../features/logs/components/LogDetails.tsx | 6 +-- .../logs/components/LogDetailsRow.tsx | 41 ++++++++++--------- .../app/features/logs/components/LogRow.tsx | 6 +-- .../app/features/logs/components/LogRows.tsx | 6 +-- .../logs/components/__mocks__/logRow.ts | 2 +- 9 files changed, 92 insertions(+), 51 deletions(-) diff --git a/public/app/features/explore/Explore.tsx b/public/app/features/explore/Explore.tsx index bbcefa5cc66..e82de044960 100644 --- a/public/app/features/explore/Explore.tsx +++ b/public/app/features/explore/Explore.tsx @@ -179,37 +179,33 @@ export class Explore extends React.PureComponent { * TODO: In the future, we would like to return active filters based the query that produced the log line. * @alpha */ - isFilterLabelActive = async (key: string, value: string) => { + isFilterLabelActive = async (key: string, value: string, refId?: string) => { if (!config.featureToggles.toggleLabelsInLogsUI) { return false; } - if (this.props.queries.length === 0) { + const query = this.props.queries.find((q) => q.refId === refId); + if (!query) { return false; } - for (const query of this.props.queries) { - const ds = await getDataSourceSrv().get(query.datasource); - if (!hasToggleableQueryFiltersSupport(ds)) { - return false; - } - if (!ds.queryHasFilter(query, { key, value })) { - return false; - } + const ds = await getDataSourceSrv().get(query.datasource); + if (hasToggleableQueryFiltersSupport(ds) && ds.queryHasFilter(query, { key, value })) { + return true; } - return true; + return false; }; /** * Used by Logs details. */ - onClickFilterLabel = (key: string, value: string) => { - this.onModifyQueries({ type: 'ADD_FILTER', options: { key, value } }); + onClickFilterLabel = (key: string, value: string, refId?: string) => { + this.onModifyQueries({ type: 'ADD_FILTER', options: { key, value } }, refId); }; /** * Used by Logs details. */ - onClickFilterOutLabel = (key: string, value: string) => { - this.onModifyQueries({ type: 'ADD_FILTER_OUT', options: { key, value } }); + onClickFilterOutLabel = (key: string, value: string, refId?: string) => { + this.onModifyQueries({ type: 'ADD_FILTER_OUT', options: { key, value } }, refId); }; onClickAddQueryRowButton = () => { @@ -220,8 +216,13 @@ export class Explore extends React.PureComponent { /** * Used by Logs details. */ - onModifyQueries = (action: QueryFixAction) => { + onModifyQueries = (action: QueryFixAction, refId?: string) => { const modifier = async (query: DataQuery, modification: QueryFixAction) => { + // This gives Logs Details support to modify the query that produced the log line. + // If not present, all queries are modified. + if (refId && refId !== query.refId) { + return query; + } const { datasource } = query; if (datasource == null) { return query; diff --git a/public/app/features/explore/Logs/Logs.tsx b/public/app/features/explore/Logs/Logs.tsx index 7c31c85270f..bab4036748d 100644 --- a/public/app/features/explore/Logs/Logs.tsx +++ b/public/app/features/explore/Logs/Logs.tsx @@ -82,8 +82,8 @@ interface Props extends Themeable2 { loadLogsVolumeData: () => void; showContextToggle?: (row?: LogRowModel) => boolean; onChangeTime: (range: AbsoluteTimeRange) => void; - onClickFilterLabel?: (key: string, value: string) => void; - onClickFilterOutLabel?: (key: string, value: string) => void; + onClickFilterLabel?: (key: string, value: string, refId?: string) => void; + onClickFilterOutLabel?: (key: string, value: string, refId?: string) => void; onStartScanning?: () => void; onStopScanning?: () => void; getRowContext?: (row: LogRowModel, origRow: LogRowModel, options: LogRowContextOptions) => Promise; @@ -95,7 +95,7 @@ interface Props extends Themeable2 { eventBus: EventBus; panelState?: ExplorePanelsState; scrollElement?: HTMLDivElement; - isFilterLabelActive?: (key: string, value: string) => Promise; + isFilterLabelActive?: (key: string, value: string, refId?: string) => Promise; logsFrames?: DataFrame[]; range: TimeRange; } diff --git a/public/app/features/explore/Logs/LogsContainer.tsx b/public/app/features/explore/Logs/LogsContainer.tsx index 31a95a4ef1b..f8f2034fe77 100644 --- a/public/app/features/explore/Logs/LogsContainer.tsx +++ b/public/app/features/explore/Logs/LogsContainer.tsx @@ -47,14 +47,14 @@ interface LogsContainerProps extends PropsFromRedux { scanRange?: RawTimeRange; syncedTimes: boolean; loadingState: LoadingState; - onClickFilterLabel: (key: string, value: string) => void; - onClickFilterOutLabel: (key: string, value: string) => void; + onClickFilterLabel: (key: string, value: string, refId?: string) => void; + onClickFilterOutLabel: (key: string, value: string, refId?: string) => void; onStartScanning: () => void; onStopScanning: () => void; eventBus: EventBus; splitOpenFn: SplitOpen; scrollElement?: HTMLDivElement; - isFilterLabelActive: (key: string, value: string) => Promise; + isFilterLabelActive: (key: string, value: string, refId?: string) => Promise; } interface LogsContainerState { diff --git a/public/app/features/logs/components/LogDetails.test.tsx b/public/app/features/logs/components/LogDetails.test.tsx index a22da725b6c..4d963491499 100644 --- a/public/app/features/logs/components/LogDetails.test.tsx +++ b/public/app/features/logs/components/LogDetails.test.tsx @@ -1,7 +1,9 @@ import { render, screen, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import React from 'react'; import { Field, LogLevel, LogRowModel, MutableDataFrame, createTheme, FieldType } from '@grafana/data'; +import { config } from '@grafana/runtime'; import { LogDetails, Props } from './LogDetails'; import { createLogRow } from './__mocks__/logRow'; @@ -58,6 +60,41 @@ describe('LogDetails', () => { expect(screen.getByLabelText('Filter for value')).toBeInTheDocument(); expect(screen.getByLabelText('Filter out value')).toBeInTheDocument(); }); + describe('With toggleLabelsInLogsUI=true', () => { + beforeAll(() => { + config.featureToggles.toggleLabelsInLogsUI = true; + }); + afterAll(() => { + config.featureToggles.toggleLabelsInLogsUI = false; + }); + it('should provide the log row to Explore filter functions', async () => { + const onClickFilterLabelMock = jest.fn(); + const onClickFilterOutLabelMock = jest.fn(); + const isFilterLabelActiveMock = jest.fn().mockResolvedValue(true); + const mockRow = createLogRow({ + logLevel: LogLevel.error, + timeEpochMs: 1546297200000, + labels: { key1: 'label1' }, + }); + + setup({ + onClickFilterLabel: onClickFilterLabelMock, + onClickFilterOutLabel: onClickFilterOutLabelMock, + isFilterLabelActive: isFilterLabelActiveMock, + row: mockRow, + }); + + expect(isFilterLabelActiveMock).toHaveBeenCalledWith('key1', 'label1', mockRow.dataFrame.refId); + + await userEvent.click(screen.getByLabelText('Filter for value in query A')); + expect(onClickFilterLabelMock).toHaveBeenCalledTimes(1); + expect(onClickFilterLabelMock).toHaveBeenCalledWith('key1', 'label1', mockRow.dataFrame.refId); + + await userEvent.click(screen.getByLabelText('Filter out value in query A')); + expect(onClickFilterOutLabelMock).toHaveBeenCalledTimes(1); + expect(onClickFilterOutLabelMock).toHaveBeenCalledWith('key1', 'label1', mockRow.dataFrame.refId); + }); + }); it('should not render filter controls when the callbacks are not provided', () => { setup( { diff --git a/public/app/features/logs/components/LogDetails.tsx b/public/app/features/logs/components/LogDetails.tsx index f4dad25f2fd..ecd529407a3 100644 --- a/public/app/features/logs/components/LogDetails.tsx +++ b/public/app/features/logs/components/LogDetails.tsx @@ -20,13 +20,13 @@ export interface Props extends Themeable2 { app?: CoreApp; styles: LogRowStyles; - onClickFilterLabel?: (key: string, value: string) => void; - onClickFilterOutLabel?: (key: string, value: string) => void; + onClickFilterLabel?: (key: string, value: string, refId?: string) => void; + onClickFilterOutLabel?: (key: string, value: string, refId?: string) => void; getFieldLinks?: (field: Field, rowIndex: number, dataFrame: DataFrame) => Array>; displayedFields?: string[]; onClickShowField?: (key: string) => void; onClickHideField?: (key: string) => void; - isFilterLabelActive?: (key: string, value: string) => Promise; + isFilterLabelActive?: (key: string, value: string, refId?: string) => Promise; } class UnThemedLogDetails extends PureComponent { diff --git a/public/app/features/logs/components/LogDetailsRow.tsx b/public/app/features/logs/components/LogDetailsRow.tsx index fc2a6cb054e..3d051399cba 100644 --- a/public/app/features/logs/components/LogDetailsRow.tsx +++ b/public/app/features/logs/components/LogDetailsRow.tsx @@ -16,8 +16,8 @@ export interface Props extends Themeable2 { disableActions: boolean; wrapLogMessage?: boolean; isLabel?: boolean; - onClickFilterLabel?: (key: string, value: string) => void; - onClickFilterOutLabel?: (key: string, value: string) => void; + onClickFilterLabel?: (key: string, value: string, refId?: string) => void; + onClickFilterOutLabel?: (key: string, value: string, refId?: string) => void; links?: Array>; getStats: () => LogLabelStatsModel[] | null; displayedFields?: string[]; @@ -25,7 +25,7 @@ export interface Props extends Themeable2 { onClickHideField?: (key: string) => void; row: LogRowModel; app?: CoreApp; - isFilterLabelActive?: (key: string, value: string) => Promise; + isFilterLabelActive?: (key: string, value: string, refId?: string) => Promise; } interface State { @@ -134,9 +134,9 @@ class UnThemedLogDetailsRow extends PureComponent { }; isFilterLabelActive = async () => { - const { isFilterLabelActive, parsedKeys, parsedValues } = this.props; + const { isFilterLabelActive, parsedKeys, parsedValues, row } = this.props; if (isFilterLabelActive) { - return await isFilterLabelActive(parsedKeys[0], parsedValues[0]); + return await isFilterLabelActive(parsedKeys[0], parsedValues[0], row.dataFrame?.refId); } return false; }; @@ -144,7 +144,7 @@ class UnThemedLogDetailsRow extends PureComponent { filterLabel = () => { const { onClickFilterLabel, parsedKeys, parsedValues, row } = this.props; if (onClickFilterLabel) { - onClickFilterLabel(parsedKeys[0], parsedValues[0]); + onClickFilterLabel(parsedKeys[0], parsedValues[0], row.dataFrame?.refId); } reportInteraction('grafana_explore_logs_log_details_filter_clicked', { @@ -157,7 +157,7 @@ class UnThemedLogDetailsRow extends PureComponent { filterOutLabel = () => { const { onClickFilterOutLabel, parsedKeys, parsedValues, row } = this.props; if (onClickFilterOutLabel) { - onClickFilterOutLabel(parsedKeys[0], parsedValues[0]); + onClickFilterOutLabel(parsedKeys[0], parsedValues[0], row.dataFrame?.refId); } reportInteraction('grafana_explore_logs_log_details_filter_clicked', { @@ -250,6 +250,7 @@ class UnThemedLogDetailsRow extends PureComponent { onClickFilterLabel, onClickFilterOutLabel, disableActions, + row, } = this.props; const { showFieldsStats, fieldStats, fieldCount } = this.state; const styles = getStyles(theme); @@ -257,6 +258,8 @@ class UnThemedLogDetailsRow extends PureComponent { const singleKey = parsedKeys == null ? false : parsedKeys.length === 1; const singleVal = parsedValues == null ? false : parsedValues.length === 1; const hasFilteringFunctionality = !disableActions && onClickFilterLabel && onClickFilterOutLabel; + const refIdTooltip = + config.featureToggles.toggleLabelsInLogsUI && row.dataFrame?.refId ? ` in query ${row.dataFrame?.refId}` : ''; const isMultiParsedValueWithNoContent = !singleVal && parsedValues != null && !parsedValues.every((val) => val === ''); @@ -275,18 +278,22 @@ class UnThemedLogDetailsRow extends PureComponent {
{hasFilteringFunctionality && ( <> - {config.featureToggles.toggleLabelsInLogsUI && ( + {config.featureToggles.toggleLabelsInLogsUI ? ( // If we are using the new label toggling, we want to use the async icon button - )} - {!config.featureToggles.toggleLabelsInLogsUI && ( + ) : ( )} - + )} {!disableActions && displayedFields && toggleFieldButton} @@ -350,10 +357,12 @@ class UnThemedLogDetailsRow extends PureComponent { interface AsyncIconButtonProps extends Pick, 'onClick'> { name: IconName; isActive(): Promise; + tooltipSuffix: string; } -const AsyncIconButton = ({ isActive, ...rest }: AsyncIconButtonProps) => { +const AsyncIconButton = ({ isActive, tooltipSuffix, ...rest }: AsyncIconButtonProps) => { const [active, setActive] = useState(false); + const tooltip = active ? 'Remove filter' : 'Filter for value'; /** * We purposely want to run this on every render to allow the active state to be updated @@ -361,13 +370,7 @@ const AsyncIconButton = ({ isActive, ...rest }: AsyncIconButtonProps) => { */ isActive().then(setActive); - return ( - - ); + return ; }; export const LogDetailsRow = withTheme2(UnThemedLogDetailsRow); diff --git a/public/app/features/logs/components/LogRow.tsx b/public/app/features/logs/components/LogRow.tsx index d3222afc6d8..3ab307a919c 100644 --- a/public/app/features/logs/components/LogRow.tsx +++ b/public/app/features/logs/components/LogRow.tsx @@ -29,8 +29,8 @@ interface Props extends Themeable2 { app?: CoreApp; displayedFields?: string[]; getRows: () => LogRowModel[]; - onClickFilterLabel?: (key: string, value: string) => void; - onClickFilterOutLabel?: (key: string, value: string) => void; + onClickFilterLabel?: (key: string, value: string, refId?: string) => void; + onClickFilterOutLabel?: (key: string, value: string, refId?: string) => void; onContextClick?: () => void; getFieldLinks?: (field: Field, rowIndex: number, dataFrame: DataFrame) => Array>; showContextToggle?: (row?: LogRowModel) => boolean; @@ -42,7 +42,7 @@ interface Props extends Themeable2 { styles: LogRowStyles; permalinkedRowId?: string; scrollIntoView?: (element: HTMLElement) => void; - isFilterLabelActive?: (key: string, value: string) => Promise; + isFilterLabelActive?: (key: string, value: string, refId?: string) => Promise; onPinLine?: (row: LogRowModel) => void; onUnpinLine?: (row: LogRowModel) => void; pinned?: boolean; diff --git a/public/app/features/logs/components/LogRows.tsx b/public/app/features/logs/components/LogRows.tsx index bce6a961241..cc1b0ccd5c3 100644 --- a/public/app/features/logs/components/LogRows.tsx +++ b/public/app/features/logs/components/LogRows.tsx @@ -38,8 +38,8 @@ export interface Props extends Themeable2 { displayedFields?: string[]; app?: CoreApp; showContextToggle?: (row?: LogRowModel) => boolean; - onClickFilterLabel?: (key: string, value: string) => void; - onClickFilterOutLabel?: (key: string, value: string) => void; + onClickFilterLabel?: (key: string, value: string, refId?: string) => void; + onClickFilterOutLabel?: (key: string, value: string, refId?: string) => void; getFieldLinks?: (field: Field, rowIndex: number, dataFrame: DataFrame) => Array>; onClickShowField?: (key: string) => void; onClickHideField?: (key: string) => void; @@ -50,7 +50,7 @@ export interface Props extends Themeable2 { onPermalinkClick?: (row: LogRowModel) => Promise; permalinkedRowId?: string; scrollIntoView?: (element: HTMLElement) => void; - isFilterLabelActive?: (key: string, value: string) => Promise; + isFilterLabelActive?: (key: string, value: string, refId?: string) => Promise; pinnedRowId?: string; containerRendered?: boolean; } diff --git a/public/app/features/logs/components/__mocks__/logRow.ts b/public/app/features/logs/components/__mocks__/logRow.ts index caee86b585d..fde85c69dc8 100644 --- a/public/app/features/logs/components/__mocks__/logRow.ts +++ b/public/app/features/logs/components/__mocks__/logRow.ts @@ -8,7 +8,7 @@ export const createLogRow = (overrides?: Partial): LogRowModel => { return { entryFieldIndex: 0, rowIndex: 0, - dataFrame: new MutableDataFrame(), + dataFrame: new MutableDataFrame({ refId: 'A', fields: [] }), uid, logLevel: LogLevel.info, entry,