Correlations: Add transformation editor #66217

gelicia · 2023-04-10T17:48:41Z

What is this feature?

This feature adds a UI that allows people to create transformations in their correlations.

Why do we need this feature?

Transformations were previously enabled via provisioning, but that does not include an intuitive interface for people to create their own transformations using Grafana's UI. This adds transformations to the existing correlations editor.

Who is this feature for?

For people who want to set up correlations in a ui instead of provisioning.

Which issue(s) does this PR fix?:

Fixes #62095

Special notes for your reviewer:

The FieldArray component is previously used in Alerting > Notification Policies > Edit to define matching labels. I believe this UI is good for instances where there will be a small enough number of rows that the repetition will not be distracting or take up too much space with redundant information, and I believe transformations is a good use case for it.
There is a lot of internal documentation meant to guide the user through a complex new feature. I left a note where we would create new transformation types to remind future developers to edit it. If we get to a place where we need to create lots of new transformations and the upkeep of the internal documentation becomes too much, we can absolutely dial it back, but for this initial launch, I think it is useful.

Please check that:

It works as expected from a user's perspective.
If this is a pre-GA feature, it is behind a feature toggle.
The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

This reverts commit 2188a3d.

…or, save values that disappear so they come back

gelicia · 2023-04-11T13:19:03Z

Could be my color blindness but I don't see any difference between rows.

Here's a side by side comparison of a correlation with a slightly dark background vs a lighter background. The issue was the variables card background was getting lost on the rows with the slightly darker background. I also added the same comparison but for dark mode

ifrost

Works really great! 🎉

I think there are two main missing bits to make it mergable:

(1) The validation for the expression field should be consistent with other fields:

Screen.Recording.2023-04-12.at.11.57.05.mov

(2) There should also be some test coverage added (at least a smoke test in CorrelationsPage.test.tsx)

Other comments are non-blockers.

ifrost · 2023-04-12T10:07:50Z

public/app/features/correlations/Forms/ConfigureCorrelationSourceForm.tsx

-});
+const getStyles = (theme: GrafanaTheme2) => {
+  // show text color at 0.05% to show with alternating background colors of interactive table
+  const decomposedContrastColor = colorManipulator.decomposeColor(theme.colors.text.primary);


I think it's a good idea to make it stand out better, but in long term we may need to rethink the design. Having all forms in the table rows may create more, similar issue. I'll cross-link this comment in #65517

packages/grafana-data/src/types/dataLink.ts

ifrost · 2023-04-12T10:09:13Z

public/app/features/correlations/Forms/TransformationsEditor.tsx

+                                  });
+                                  const newValueDetails = getSupportedTransTypeDetails(value.value);
+
+                                  if (newValueDetails.showExpression) {


The issue is that I would like to prevent data loss as much as possible for this.

Fair enough. It's quite complex logic and it'd be good to add test coverage for it.

public/app/features/correlations/Forms/TransformationsEditor.tsx

ifrost · 2023-04-12T10:11:02Z

public/app/features/correlations/types.ts

@@ -8,6 +8,11 @@ export type GetCorrelationsResponse = Correlation[];

 type CorrelationConfigType = 'query';

+export enum TransformationType {


Could we use SupportedTransformationType from @grafana/data instead?

Ooops - yep! Fixed

ifrost · 2023-04-12T10:12:47Z

public/app/features/correlations/Forms/TransformationsEditor.tsx

+                          rules={{ required: { value: true, message: 'Please select a transformation type' } }}
+                        />
+                      </Field>
+                      <Field


New fields don't have any aria-labels, probably worth adding some.

Many were added to correspond to the tests but I double checked and made sure all buttons and fields have an aria label

ifrost · 2023-04-12T10:14:51Z

public/app/features/correlations/Forms/TransformationsEditor.tsx

+                            <Select
+                              {...field}
+                              onChange={(value) => {
+                                if (!readOnly) {


This looks very confusing in read-only mode. Not a blocker for now but I think we should run it by UX. I'll also mention it in #65517. I think making the description accessible in read-only mode was suggested by @radiorental, but is it the right way of doing it?

…formation delete/add bug

…na/transformations-editor

gelicia · 2023-04-13T18:23:24Z

public/app/features/correlations/Forms/TransformationsEditor.tsx

+                              return (
+                                <Select
+                                  {...field}
+                                  value={fieldVal.type}


@ifrost this is to fix the bug I talked about.

Replication steps:

Have a transformation saved already. Refresh to clean slate the state.

Open the correlation and go to the transformation. Delete it

Add a new transformation. The select will be pre-filled with the original transformation

To see what the deal is, if you add a console.log for field in the InputControl render and fieldVal in the fields.map above, you will see the InputControl field still persists this value, as it must grab it from some state that does not get affected by remove. If there's a more obvious solution here, please let me know :)

ifrost

Validation and tests look good 👍 There's still a few comments I left last time that would be good to address.

gelicia · 2023-04-14T19:09:25Z

Found a bug creating a whole new correlation where it gives the "Please select a transformation type" when there is one selected. Will fix and add a test for this.

Edit: I only saw this once and was never able to recreate it again. I added creating a transformation in the test for creating a correlation, it should catch this and fail if an error message pops up that prevents saving.

Elfo404 · 2023-04-15T08:08:54Z

public/app/features/correlations/Forms/TransformationsEditor.tsx

+                        <Field
+                          label={
+                            <Stack gap={0.5}>
+                              <Label htmlFor={`config.transformations.${index}.type`}>Type</Label>


we need to use the same id as inputId prop in the Select at line 81 otherwise that select is going to be unlabeled.

moreover, we should not use the index here as it causes this label to target all transformations being edited with the same index:

Screen.Recording.2023-04-15.at.09.06.57.mov

we could use htmlFor={`config.transformations.${fieldVal.id}.type`} here and inputId={`config.transformations.${fieldVal.id}.type`} in the select. and by doing that you should be able to leverage this label element to describe the input instead of using an explicit aria-label on the select

Suggested change

<Label htmlFor={`config.transformations.${index}.type`}>Type</Label>

<Label htmlFor={`config.transformations.${fieldVal.id}.type`}>Type</Label>

Elfo404 · 2023-04-15T08:12:18Z

public/app/features/correlations/Forms/TransformationsEditor.tsx

+                        <Field
+                          label={
+                            <Stack gap={0.5}>
+                              <Label>Field</Label>


we should add an htmlFor here:

Suggested change

<Label>Field</Label>

<Label htmlFor={`config.transformations.${fieldVal.id}.field`}>Field</Label>

Elfo404 · 2023-04-15T08:12:54Z

public/app/features/correlations/Forms/TransformationsEditor.tsx

+                            readOnly={readOnly}
+                            defaultValue={fieldVal.field}
+                            label="field"
+                            aria-label="field"


to pair with the above comment:

Suggested change

aria-label="field"

id={`config.transformations.${fieldVal.id}.field`}

Elfo404 · 2023-04-15T08:14:31Z

public/app/features/correlations/Forms/TransformationsEditor.tsx

+                                  }}
+                                  options={transformOptions}
+                                  width={25}
+                                  aria-label="Type"


Suggested change

aria-label="Type"

inputId={`config.transformations.${fieldVal.id}.type`}

public/app/features/correlations/Forms/TransformationsEditor.tsx

Elfo404 · 2023-04-15T08:32:46Z

public/app/features/correlations/Forms/TransformationsEditor.tsx

+  return Object.keys(SupportedTransformationType).map((key) => {
+    const transType = getSupportedTransTypeDetails(
+      SupportedTransformationType[key as keyof typeof SupportedTransformationType]
+    );


i think you can do this instead so you don't have to do any ugly type assertion:

Suggested change

return Object.keys(SupportedTransformationType).map((key) => {

const transType = getSupportedTransTypeDetails(

SupportedTransformationType[key as keyof typeof SupportedTransformationType]

);

return Object.values(SupportedTransformationType).map((key) => {

const transType = getSupportedTransTypeDetails(key);

or probably better:

Suggested change

return Object.keys(SupportedTransformationType).map((key) => {

const transType = getSupportedTransTypeDetails(

SupportedTransformationType[key as keyof typeof SupportedTransformationType]

);

return Object.values(SupportedTransformationType).map((transformationType) => {

const transType = getSupportedTransTypeDetails(transformationType);

@Elfo404 good call, this worked great!

…na/transformations-editor

Elfo404

LGTM! 🚀

gelicia and others added 11 commits April 10, 2023 12:35

There was an attempt

faab673

Change disabled state based on transformation type

5ecf7b1

Add validation to transformation type

04e6b2c

Revert "Add validation to transformation type"

bbcb42c

This reverts commit 2188a3d.

Add validation to transformation type

cd8af00

Move transformations editor to a separate file

c8e0957

Make name more descriptive

36dd4f4

Ensure type dropdown has always the same width

7c0096a

Add tooltips around transformation options

69a5c29

Slight style changes

ac9759e

Remove autofocus on append, integrate read only to transformationedit…

bf8a3c9

…or, save values that disappear so they come back

gelicia added add to changelog no-backport Skip backport of PR labels Apr 10, 2023

gelicia added this to the 10.0.0 milestone Apr 10, 2023

gelicia marked this pull request as ready for review April 10, 2023 17:48

gelicia requested review from joshhunt and a team as code owners April 10, 2023 17:48

gelicia requested review from JoaoSilvaGrafana, leventebalogh, sakjur, papagian and yangkb09 and removed request for a team April 10, 2023 17:48

grafanabot added the area/explore label Apr 10, 2023

Remove unused empty form object

2aac59f

gelicia removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Apr 11, 2023

Fix bug about transformation’s values saved in memory

73ea086

gelicia requested a review from ifrost April 11, 2023 19:10

ifrost requested changes Apr 12, 2023

View reviewed changes

grafanabot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Apr 12, 2023

gelicia added 6 commits April 13, 2023 09:09

Better validation formatting for expression

14370fd

Add labels and (for now) non working test, attempt to fix saved trans…

64193b6

…formation delete/add bug

Fix datalink comment

18b83e8

Merge branch 'main' of https://github.com/grafana/grafana into kristi…

20e044a

…na/transformations-editor

Remove fancy CSS due to background change

ecbb39c

Fix deleting saved transformation bug, finish tests

285b80e

gelicia commented Apr 13, 2023

View reviewed changes

gelicia requested a review from ifrost April 13, 2023 18:24

ifrost approved these changes Apr 14, 2023

View reviewed changes

gelicia added 2 commits April 14, 2023 08:46

Consolidate transformation types

dc5b219

Double check aria labels

bdcfa27

gelicia requested a review from Elfo404 April 14, 2023 14:09

Elfo404 requested changes Apr 15, 2023

View reviewed changes

Change aria labels, fix tests

bfc25b3

gelicia requested a review from Elfo404 April 17, 2023 15:53

gelicia added 2 commits April 17, 2023 11:28

Add a transformation with the create correlation test

58401e3

Merge branch 'main' of https://github.com/grafana/grafana into kristi…

8c6c57b

…na/transformations-editor

Elfo404 approved these changes Apr 18, 2023

View reviewed changes

gelicia merged commit 9d69d31 into main Apr 18, 2023
15 of 16 checks passed

gelicia deleted the kristina/transformations-editor branch April 18, 2023 12:17

zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correlations: Add transformation editor #66217

Correlations: Add transformation editor #66217

gelicia commented Apr 10, 2023 •

edited

gelicia commented Apr 11, 2023 •

edited

ifrost left a comment

ifrost Apr 12, 2023

ifrost Apr 12, 2023

ifrost Apr 12, 2023

gelicia Apr 14, 2023

ifrost Apr 12, 2023

gelicia Apr 14, 2023

ifrost Apr 12, 2023

gelicia Apr 13, 2023

ifrost left a comment

gelicia commented Apr 14, 2023 •

edited

Elfo404 Apr 15, 2023

Elfo404 Apr 15, 2023

Elfo404 Apr 15, 2023

Elfo404 Apr 15, 2023

Elfo404 Apr 15, 2023

Elfo404 Apr 15, 2023

gelicia Apr 17, 2023

Elfo404 left a comment

		@@ -8,6 +8,11 @@ export type GetCorrelationsResponse = Correlation[];

		type CorrelationConfigType = 'query';

		export enum TransformationType {

	<Label htmlFor={`config.transformations.${index}.type`}>Type</Label>
	<Label htmlFor={`config.transformations.${fieldVal.id}.type`}>Type</Label>

	<Label>Field</Label>
	<Label htmlFor={`config.transformations.${fieldVal.id}.field`}>Field</Label>

	aria-label="field"
	id={`config.transformations.${fieldVal.id}.field`}

	aria-label="Type"
	inputId={`config.transformations.${fieldVal.id}.type`}

Correlations: Add transformation editor #66217

Correlations: Add transformation editor #66217

Conversation

gelicia commented Apr 10, 2023 • edited

gelicia commented Apr 11, 2023 • edited

ifrost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifrost left a comment

Choose a reason for hiding this comment

gelicia commented Apr 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elfo404 left a comment

Choose a reason for hiding this comment

gelicia commented Apr 10, 2023 •

edited

gelicia commented Apr 11, 2023 •

edited

gelicia commented Apr 14, 2023 •

edited