From 9d69d3173f735c145f1601328e7c5f42795ac7d6 Mon Sep 17 00:00:00 2001 From: Kristina Date: Tue, 18 Apr 2023 07:17:30 -0500 Subject: [PATCH] Correlations: Add transformation editor (#66217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * There was an attempt * Change disabled state based on transformation type * Add validation to transformation type * Revert "Add validation to transformation type" This reverts commit 2188a3d9a93aec5eeafcdd40510391ba1a53671a. * Add validation to transformation type * Move transformations editor to a separate file * Make name more descriptive * Ensure type dropdown has always the same width * Add tooltips around transformation options * Slight style changes * Remove autofocus on append, integrate read only to transformationeditor, save values that disappear so they come back * Remove yaml changes * Have variable background color work with alternating colors on different themes * Make expression required for regular expressions * Remove unused empty form object * Fix bug about transformation’s values saved in memory * Better validation formatting for expression * Add labels and (for now) non working test, attempt to fix saved transformation delete/add bug * Fix datalink comment * Remove fancy CSS due to background change * Fix deleting saved transformation bug, finish tests * Consolidate transformation types * Double check aria labels * Change aria labels, fix tests * Add a transformation with the create correlation test --------- Co-authored-by: Piotr Jamróz --- packages/grafana-data/src/types/dataLink.ts | 11 +- packages/grafana-ui/src/types/forms.ts | 4 +- .../correlations/CorrelationsPage.test.tsx | 89 ++++- .../Forms/ConfigureCorrelationSourceForm.tsx | 31 +- .../Forms/TransformationsEditor.tsx | 315 ++++++++++++++++++ .../app/features/correlations/Forms/types.ts | 8 + .../features/correlations/transformations.ts | 6 +- .../app/features/explore/utils/links.test.ts | 20 +- 8 files changed, 447 insertions(+), 37 deletions(-) create mode 100644 public/app/features/correlations/Forms/TransformationsEditor.tsx diff --git a/packages/grafana-data/src/types/dataLink.ts b/packages/grafana-data/src/types/dataLink.ts index 0e11beec147..c45a3ed7e7b 100644 --- a/packages/grafana-data/src/types/dataLink.ts +++ b/packages/grafana-data/src/types/dataLink.ts @@ -52,15 +52,20 @@ export interface DataLink { origin?: DataLinkConfigOrigin; } -/** @internal */ -export enum SupportedTransformationTypes { +/** + * We provide tooltips with information about these to guide the user, please + * check for validity when adding more transformation types. + * + * @internal + */ +export enum SupportedTransformationType { Regex = 'regex', Logfmt = 'logfmt', } /** @internal */ export interface DataLinkTransformationConfig { - type: SupportedTransformationTypes; + type: SupportedTransformationType; field?: string; expression?: string; mapValue?: string; diff --git a/packages/grafana-ui/src/types/forms.ts b/packages/grafana-ui/src/types/forms.ts index 77b82c3f2c1..c5451524c95 100644 --- a/packages/grafana-ui/src/types/forms.ts +++ b/packages/grafana-ui/src/types/forms.ts @@ -1,4 +1,4 @@ -import { UseFormReturn, FieldValues, FieldErrors } from 'react-hook-form'; +import { UseFormReturn, FieldValues, FieldErrors, FieldArrayMethodProps } from 'react-hook-form'; export type { SubmitHandler as FormsOnSubmit, FieldErrors as FormFieldErrors } from 'react-hook-form'; export type FormAPI = Omit, 'trigger' | 'handleSubmit'> & { @@ -9,7 +9,7 @@ type FieldArrayValue = Partial | Array>; export interface FieldArrayApi { fields: Array>; - append: (value: FieldArrayValue) => void; + append: (value: FieldArrayValue, options?: FieldArrayMethodProps) => void; prepend: (value: FieldArrayValue) => void; remove: (index?: number | number[]) => void; swap: (indexA: number, indexB: number) => void; diff --git a/public/app/features/correlations/CorrelationsPage.test.tsx b/public/app/features/correlations/CorrelationsPage.test.tsx index abfdc10a78c..051841c88c1 100644 --- a/public/app/features/correlations/CorrelationsPage.test.tsx +++ b/public/app/features/correlations/CorrelationsPage.test.tsx @@ -2,12 +2,13 @@ import { render, waitFor, screen, fireEvent, within, Matcher, getByRole } from ' import userEvent from '@testing-library/user-event'; import { merge, uniqueId } from 'lodash'; import React from 'react'; +import { openMenu } from 'react-select-event'; import { Observable } from 'rxjs'; import { TestProvider } from 'test/helpers/TestProvider'; import { MockDataSourceApi } from 'test/mocks/datasource_srv'; import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock'; -import { DataSourcePluginMeta } from '@grafana/data'; +import { DataSourcePluginMeta, SupportedTransformationType } from '@grafana/data'; import { BackendSrv, setDataSourceSrv, BackendSrvRequest, reportInteraction } from '@grafana/runtime'; import { contextSrv } from 'app/core/services/context_srv'; import { configureStore } from 'app/store/configureStore'; @@ -274,6 +275,14 @@ describe('CorrelationsPage', () => { await userEvent.clear(screen.getByRole('textbox', { name: /results field/i })); await userEvent.type(screen.getByRole('textbox', { name: /results field/i }), 'Line'); + + // add transformation + await userEvent.click(screen.getByRole('button', { name: /add transformation/i })); + const typeFilterSelect = screen.getAllByLabelText('Type'); + openMenu(typeFilterSelect[0]); + await userEvent.click(screen.getByText('Regular expression')); + await userEvent.type(screen.getByLabelText(/expression/i), 'test expression'); + await userEvent.click(await screen.findByRole('button', { name: /add$/i })); expect(mocks.reportInteraction).toHaveBeenLastCalledWith('grafana_correlations_added'); @@ -342,7 +351,14 @@ describe('CorrelationsPage', () => { targetUID: 'loki', uid: '1', label: 'Some label', - config: { field: 'line', target: {}, type: 'query' }, + config: { + field: 'line', + target: {}, + type: 'query', + transformations: [ + { type: SupportedTransformationType.Regex, expression: 'url=http[s]?://(S*)', mapValue: 'path' }, + ], + }, }, { sourceUID: 'prometheus', @@ -472,12 +488,64 @@ describe('CorrelationsPage', () => { await userEvent.click(screen.getByRole('button', { name: /next$/i })); await userEvent.click(screen.getByRole('button', { name: /next$/i })); + await userEvent.click(screen.getByRole('button', { name: /save$/i })); expect(await screen.findByRole('cell', { name: /edited label$/i })).toBeInTheDocument(); expect(mocks.reportInteraction).toHaveBeenLastCalledWith('grafana_correlations_edited'); }); + + it('correctly edits transformations', async () => { + // wait for table to appear + await screen.findByRole('table'); + + const tableRows = queryRowsByCellValue('Source', 'loki'); + + const rowExpanderButton = within(tableRows[0]).getByRole('button', { name: /toggle row expanded/i }); + await userEvent.click(rowExpanderButton); + + await userEvent.click(screen.getByRole('button', { name: /next$/i })); + await userEvent.click(screen.getByRole('button', { name: /next$/i })); + + // select Logfmt, be sure expression field is disabled + let typeFilterSelect = screen.getAllByLabelText('Type'); + openMenu(typeFilterSelect[0]); + await userEvent.click(screen.getByText('Logfmt')); + + let expressionInput = screen.queryByLabelText(/expression/i); + expect(expressionInput).toBeInTheDocument(); + expect(expressionInput).toBeDisabled(); + + // select Regex, be sure expression field is not disabled and contains the former expression + openMenu(typeFilterSelect[0]); + await userEvent.click(screen.getByText('Regular expression', { selector: 'span' })); + expressionInput = screen.queryByLabelText(/expression/i); + expect(expressionInput).toBeInTheDocument(); + expect(expressionInput).toBeEnabled(); + expect(expressionInput).toHaveAttribute('value', 'url=http[s]?://(S*)'); + + // select Logfmt, delete, then add a new one to be sure the value is blank + openMenu(typeFilterSelect[0]); + await userEvent.click(screen.getByText('Logfmt')); + await userEvent.click(screen.getByRole('button', { name: /remove transformation/i })); + expressionInput = screen.queryByLabelText(/expression/i); + expect(expressionInput).not.toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: /add transformation/i })); + typeFilterSelect = screen.getAllByLabelText('Type'); + openMenu(typeFilterSelect[0]); + await userEvent.click(screen.getByText('Regular expression')); + expressionInput = screen.queryByLabelText(/expression/i); + expect(expressionInput).toBeInTheDocument(); + expect(expressionInput).toBeEnabled(); + expect(expressionInput).not.toHaveValue('url=http[s]?://(S*)'); + await userEvent.click(screen.getByRole('button', { name: /save$/i })); + expect(screen.getByText('Please define an expression')).toBeInTheDocument(); + await userEvent.type(screen.getByLabelText(/expression/i), 'test expression'); + await userEvent.click(screen.getByRole('button', { name: /save$/i })); + expect(mocks.reportInteraction).toHaveBeenLastCalledWith('grafana_correlations_edited'); + }); }); describe('Read only correlations', () => { @@ -487,7 +555,12 @@ describe('CorrelationsPage', () => { targetUID: 'loki', uid: '1', label: 'Some label', - config: { field: 'line', target: {}, type: 'query' }, + config: { + field: 'line', + target: {}, + type: 'query', + transformations: [{ type: SupportedTransformationType.Regex, expression: '(?:msg)=' }], + }, }, ]; @@ -532,6 +605,16 @@ describe('CorrelationsPage', () => { expect(descriptionInput).toBeInTheDocument(); expect(descriptionInput).toHaveAttribute('readonly'); + await userEvent.click(screen.getByRole('button', { name: /next$/i })); + await userEvent.click(screen.getByRole('button', { name: /next$/i })); + + // expect the transformation to exist but be read only + const expressionInput = screen.queryByLabelText(/expression/i); + expect(expressionInput).toBeInTheDocument(); + expect(expressionInput).toHaveAttribute('readonly'); + expect(screen.queryByRole('button', { name: 'add transformation' })).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'remove transformation' })).not.toBeInTheDocument(); + // we don't expect the save button to be rendered expect(screen.queryByRole('button', { name: 'save' })).not.toBeInTheDocument(); }); diff --git a/public/app/features/correlations/Forms/ConfigureCorrelationSourceForm.tsx b/public/app/features/correlations/Forms/ConfigureCorrelationSourceForm.tsx index f60d62a118c..4588e881a7b 100644 --- a/public/app/features/correlations/Forms/ConfigureCorrelationSourceForm.tsx +++ b/public/app/features/correlations/Forms/ConfigureCorrelationSourceForm.tsx @@ -9,6 +9,7 @@ import { getDatasourceSrv } from 'app/features/plugins/datasource_srv'; import { getVariableUsageInfo } from '../../explore/utils/links'; +import { TransformationsEditor } from './TransformationsEditor'; import { useCorrelationsFormContext } from './correlationsFormContext'; import { getInputId } from './utils'; @@ -33,12 +34,11 @@ export const ConfigureCorrelationSourceForm = () => { const variables = getVariableUsageInfo(currentTargetQuery, {}).variables.map( (variable) => variable.variableName + (variable.fieldPath ? `.${variable.fieldPath}` : '') ); - return ( <>

- Links are displayed with results of the selected origin source data. They shown along with the value of the + Links are displayed with results of the selected origin source data. They show along with the value of the provided results field.

{ readOnly={readOnly} /> - {variables.length > 0 && ( Variables used in the target query -
- You have used following variables in the target query:{' '} - {variables.map((name, i) => ( - - {name} - {i < variables.length - 1 ? ', ' : ''} - - ))} -
-
- A data point needs to provide values to all variables as fields or as transformations output to make the - correlation button appear in the visualization. -
+ You have used following variables in the target query:{' '} + {variables.map((name, i) => ( + + {name} + {i < variables.length - 1 ? ', ' : ''} + + ))} +
A data point needs to provide values to all variables as fields or as transformations output to make + the correlation button appear in the visualization. +
+ Note: Not every variable needs to be explicitly defined below. A transformation such as{' '} + logfmt will create variables for every key/value pair. )} +
); diff --git a/public/app/features/correlations/Forms/TransformationsEditor.tsx b/public/app/features/correlations/Forms/TransformationsEditor.tsx new file mode 100644 index 00000000000..a388bf55505 --- /dev/null +++ b/public/app/features/correlations/Forms/TransformationsEditor.tsx @@ -0,0 +1,315 @@ +import { css } from '@emotion/css'; +import { compact, fill } from 'lodash'; +import React, { useState } from 'react'; +import { useFormContext } from 'react-hook-form'; + +import { GrafanaTheme2, SupportedTransformationType } from '@grafana/data'; +import { Stack } from '@grafana/experimental'; +import { + Button, + Field, + FieldArray, + Icon, + IconButton, + Input, + InputControl, + Label, + Select, + Tooltip, + useStyles2, +} from '@grafana/ui'; + +type Props = { readOnly: boolean }; + +const getStyles = (theme: GrafanaTheme2) => ({ + heading: css` + font-size: ${theme.typography.h5.fontSize}; + font-weight: ${theme.typography.fontWeightRegular}; + `, + // set fixed position from the top instead of centring as the container + // may get bigger when the for is invalid + removeButton: css` + margin-top: 25px; + `, +}); + +export const TransformationsEditor = (props: Props) => { + const { control, formState, register, setValue, watch, getValues } = useFormContext(); + const { readOnly } = props; + const [keptVals, setKeptVals] = useState>([]); + + const styles = useStyles2(getStyles); + + const transformOptions = getTransformOptions(); + return ( + <> + + + {({ fields, append, remove }) => ( + <> + +
Transformations
+ {fields.length === 0 &&
No transformations defined.
} + {fields.length > 0 && ( +
+ {fields.map((fieldVal, index) => { + return ( + + + + +

The type of transformation that will be applied to the source data.

+
+ } + > + + + + } + invalid={!!formState.errors?.config?.transformations?.[index]?.type} + error={formState.errors?.config?.transformations?.[index]?.type?.message} + validationMessageHorizontalOverflow={true} + > + { + // input control field is not manipulated with remove, use value from control + return ( + + + + + +

+ Required for regular expression. The expression the transformation will use. + Logfmt does not use further specifications. +

+ + } + > + + + + } + invalid={!!formState.errors?.config?.transformations?.[index]?.expression} + error={formState.errors?.config?.transformations?.[index]?.expression?.message} + > + + + + + +

+ Optional. Defines the name of the variable. This is currently only valid for + regular expressions with a single, unnamed capture group. +

+ + } + > + + + + } + > + + + {!readOnly && ( +
+ { + remove(index); + const keptValsCopy: Array<{ expression?: string; mapValue?: string } | undefined> = [ + ...keptVals, + ]; + keptValsCopy[index] = undefined; + setKeptVals(compact(keptValsCopy)); + }} + ariaLabel="Remove transformation" + > + Remove + +
+ )} + + ); + })} + + )} + {!readOnly && ( + + )} + + + )} + + + ); +}; + +interface SupportedTransformationTypeDetails { + label: string; + value: string; + description?: string; + showExpression: boolean; + showMapValue: boolean; + requireExpression?: boolean; +} + +function getSupportedTransTypeDetails(transType: SupportedTransformationType): SupportedTransformationTypeDetails { + switch (transType) { + case SupportedTransformationType.Logfmt: + return { + label: 'Logfmt', + value: SupportedTransformationType.Logfmt, + description: 'Parse provided field with logfmt to get variables', + showExpression: false, + showMapValue: false, + }; + case SupportedTransformationType.Regex: + return { + label: 'Regular expression', + value: SupportedTransformationType.Regex, + description: + 'Field will be parsed with regex. Use named capture groups to return multiple variables, or a single unnamed capture group to add variable to named map value.', + showExpression: true, + showMapValue: true, + requireExpression: true, + }; + default: + return { label: transType, value: transType, showExpression: false, showMapValue: false }; + } +} + +const getTransformOptions = () => { + return Object.values(SupportedTransformationType).map((transformationType) => { + const transType = getSupportedTransTypeDetails(transformationType); + return { + label: transType.label, + value: transType.value, + description: transType.description, + }; + }); +}; diff --git a/public/app/features/correlations/Forms/types.ts b/public/app/features/correlations/Forms/types.ts index 368c83f6cee..d3e10e581c4 100644 --- a/public/app/features/correlations/Forms/types.ts +++ b/public/app/features/correlations/Forms/types.ts @@ -1,3 +1,5 @@ +import { SupportedTransformationType } from '@grafana/data'; + import { CorrelationConfig } from '../types'; export interface FormDTO { @@ -9,3 +11,9 @@ export interface FormDTO { } export type EditFormDTO = Omit; + +export type TransformationDTO = { + type: SupportedTransformationType; + expression?: string; + mapValue?: string; +}; diff --git a/public/app/features/correlations/transformations.ts b/public/app/features/correlations/transformations.ts index 86d9c98cb5b..6754d299545 100644 --- a/public/app/features/correlations/transformations.ts +++ b/public/app/features/correlations/transformations.ts @@ -1,6 +1,6 @@ import logfmt from 'logfmt'; -import { ScopedVars, DataLinkTransformationConfig, SupportedTransformationTypes } from '@grafana/data'; +import { ScopedVars, DataLinkTransformationConfig, SupportedTransformationType } from '@grafana/data'; import { safeStringifyValue } from 'app/core/utils/explore'; export const getTransformationVars = ( @@ -10,7 +10,7 @@ export const getTransformationVars = ( ): ScopedVars => { let transformationScopedVars: ScopedVars = {}; let transformVal: { [key: string]: string | boolean | null | undefined } = {}; - if (transformation.type === SupportedTransformationTypes.Regex && transformation.expression) { + if (transformation.type === SupportedTransformationType.Regex && transformation.expression) { const regexp = new RegExp(transformation.expression, 'gi'); const matches = fieldValue.matchAll(regexp); for (const match of matches) { @@ -20,7 +20,7 @@ export const getTransformationVars = ( transformVal[transformation.mapValue || fieldName] = match[1] || match[0]; } } - } else if (transformation.type === SupportedTransformationTypes.Logfmt) { + } else if (transformation.type === SupportedTransformationType.Logfmt) { transformVal = logfmt.parse(fieldValue); } diff --git a/public/app/features/explore/utils/links.test.ts b/public/app/features/explore/utils/links.test.ts index 86f84abd7d1..f8bd1fa9e2c 100644 --- a/public/app/features/explore/utils/links.test.ts +++ b/public/app/features/explore/utils/links.test.ts @@ -8,7 +8,7 @@ import { Field, FieldType, InterpolateFunction, - SupportedTransformationTypes, + SupportedTransformationType, TimeRange, toDataFrame, } from '@grafana/data'; @@ -285,8 +285,8 @@ describe('explore links utils', () => { datasourceUid: 'uid_1', datasourceName: 'test_ds', transformations: [ - { type: SupportedTransformationTypes.Logfmt }, - { type: SupportedTransformationTypes.Regex, expression: 'host=(dev|prod)', mapValue: 'environment' }, + { type: SupportedTransformationType.Logfmt }, + { type: SupportedTransformationType.Regex, expression: 'host=(dev|prod)', mapValue: 'environment' }, ], }, }; @@ -334,8 +334,8 @@ describe('explore links utils', () => { datasourceUid: 'uid_1', datasourceName: 'test_ds', transformations: [ - { type: SupportedTransformationTypes.Regex, expression: 'fieldA=(asparagus|broccoli)' }, - { type: SupportedTransformationTypes.Regex, expression: 'fieldB=(apple|banana)' }, + { type: SupportedTransformationType.Regex, expression: 'fieldA=(asparagus|broccoli)' }, + { type: SupportedTransformationType.Regex, expression: 'fieldB=(apple|banana)' }, ], }, }; @@ -375,7 +375,7 @@ describe('explore links utils', () => { query: { query: 'http_requests{app=${application} isOnline=${online}}' }, datasourceUid: 'uid_1', datasourceName: 'test_ds', - transformations: [{ type: SupportedTransformationTypes.Logfmt }], + transformations: [{ type: SupportedTransformationType.Logfmt }], }, }; @@ -414,7 +414,7 @@ describe('explore links utils', () => { query: { query: 'http_requests{app=${application}}' }, datasourceUid: 'uid_1', datasourceName: 'test_ds', - transformations: [{ type: SupportedTransformationTypes.Logfmt, field: 'fieldNamedInTransformation' }], + transformations: [{ type: SupportedTransformationType.Logfmt, field: 'fieldNamedInTransformation' }], }, }; @@ -468,7 +468,7 @@ describe('explore links utils', () => { datasourceName: 'test_ds', transformations: [ { - type: SupportedTransformationTypes.Regex, + type: SupportedTransformationType.Regex, expression: '(?=.*(?(grafana|loki)))(?=.*(?(dev|prod)))', }, ], @@ -546,7 +546,7 @@ describe('explore links utils', () => { query: { query: 'http_requests{app=${application} env=${diffVar}}' }, datasourceUid: 'uid_1', datasourceName: 'test_ds', - transformations: [{ type: SupportedTransformationTypes.Logfmt }], + transformations: [{ type: SupportedTransformationType.Logfmt }], }, }; @@ -571,7 +571,7 @@ describe('explore links utils', () => { query: { query: 'http_requests{app=test}' }, datasourceUid: 'uid_1', datasourceName: 'test_ds', - transformations: [{ type: SupportedTransformationTypes.Logfmt }], + transformations: [{ type: SupportedTransformationType.Logfmt }], }, };