feat(slo): slo edit form improvements #148419

kdelemme · 2023-01-04T19:56:09Z

📝 Summary

Resolves #148311
Resolves #148308
Resolves #148254
Part of #148306

This PR changes the index selector logic to allow partial index selection with * suffix. The suggestion is still broken when using the * prefix though. We need to iterate on this or use another component.

This PR also adds a new CreateSLOInput type that is the Output of the CreateSLOParams schema. Meaning it's an object with only primitive values that is expected to be received by the API route handler. Which then is validated and parsed into a CreateSLOParams type by io-ts and used in the application service.

Finally, this PR fixes the form when using a timeslices budgeting method, and correctly handles the edit flow of an existing SLO.

🥼 Manual testing

Run the branch locally and go into the SLOs page. You can play with the create form and then edit any existing SLO from there.

🎥 Recording

create.slo.mov

edit.slo.mov

elasticmachine · 2023-01-04T19:58:58Z

Pinging @elastic/actionable-observability (Team: Actionable Observability)

kdelemme · 2023-01-05T14:20:56Z

x-pack/plugins/observability/server/services/slo/update_slo.ts

@@ -41,7 +42,7 @@ export class UpdateSLO {

  private updateSLO(originalSlo: SLO, params: UpdateSLOParams) {
    let hasBreakingChange = false;
-    const updatedSlo: SLO = Object.assign({}, originalSlo, params, { updated_at: new Date() });
+    const updatedSlo: SLO = merge({}, originalSlo, params, { updatedAt: new Date() });


💬 lodash's merge is better than a simple Object.assign as it merge recursively the objects

kdelemme · 2023-01-05T14:21:49Z

x-pack/plugins/observability/public/pages/slos/components/slo_list_search_filter_sort_bar.tsx

@@ -27,10 +27,10 @@ export interface SloListSearchFilterSortBarProps {
  onChangeIndicatorTypeFilter: (filter: FilterType[]) => void;
 }

-export type SortType = 'name' | 'indicator_type';
+export type SortType = 'name' | 'indicatorType';


💬 Missed part of a previous refactoring from snake_case to camelCase. The values have been updated to camelCase as well.

kdelemme · 2023-01-05T14:22:19Z

x-pack/plugins/observability/public/pages/slo_edit/index.test.tsx

@@ -163,19 +165,58 @@ describe('SLO Edit Page', () => {
      );
    });

-    it('renders the SLO Edit page with prefilled form values if sloId route param is passed', async () => {
+    it('calls the createSlo hook if all required values are filled in', async () => {


💬 Only new test is here. Others have been slightly modified to fit the new use case (edit)

kdelemme · 2023-01-05T14:24:33Z

x-pack/plugins/observability/public/pages/slo_edit/helpers/process_slo_form_values.ts

+  };
+}
+
+export function transformValuesToUpdateSLOInput(values: CreateSLOInput): UpdateSLOInput {


💬 Duplication on purpose. They are not the same type even if UpdateSLOInput is included in (i.e. ⊂) CreateSLOInput

kdelemme · 2023-01-05T14:25:40Z

x-pack/plugins/observability/public/pages/slo_edit/helpers/process_slo_form_values.ts

  if (!values) return undefined;

  return {
-    ...values,
+    ...omit(values, ['id', 'revision', 'createdAt', 'updatedAt', 'summary']),


💬 These fields are not used by the form and must not be present in the payload for the create or update API.

Does it mean whatever data we pass to the form, will be sent to the API?

If sending this information can create an issue on the API side, I am wondering if it make sense to omit these fields there as well 🤔

@maryam-saeidi

I am wondering if it make sense to omit these fields there as well 🤔

omit these fields where?

On the BE side.

Maybe I am missing something but I thought for fields like 'createdAt', 'updatedAt' we always want to set them on the BE side, so I thought maybe we can ignore these fields even if FE sends them to make sure it will not create an issue.

kdelemme · 2023-01-05T14:26:14Z

x-pack/plugins/observability/public/pages/slo_edit/constants.ts

@@ -35,7 +53,8 @@ export const SLO_EDIT_FORM_DEFAULT_VALUES: CreateSLOParams = {
    },
  },
  timeWindow: {
-    duration: TIMEWINDOW_OPTIONS[0].value as any, // Get this to be a proper Duration
+    duration:


💬 Now that we use CreateSLOInput, the types are only primitives, so no more casting as any necessary

kdelemme · 2023-01-05T17:57:09Z

x-pack/plugins/observability/public/pages/slo_edit/index.test.tsx

@@ -163,19 +165,61 @@ describe('SLO Edit Page', () => {
      );
    });

-    it('renders the SLO Edit page with prefilled form values if sloId route param is passed', async () => {
+    it.skip('calls the createSlo hook if all required values are filled in', async () => {


💬 Disabled for now as it takes 4s to wait for the submit button to be enabled... And I can't figure out why.

maryam-saeidi

Nice Job! 👏🏻

Tested locally, I've added some comments in the code and had some questions in general that probably are not in the scope of this ticket just noticed them while reviewing:

I didn't see a loader for the SLO list page
Create/Edit SLO
1. Index input is invalid at the start, even before user started filling the form
2. Create SLO is disabled but it is not clear why
  1. It would be better to have it enabled but when clicked, we show invalid inputs
3. I saw in creating a rule in security, there is a possibility to only show one step at a time, maybe it can be useful here as well
4. Since we only have KQL input, can we disable clicking on the KQL part of the input?
5. I think it will be helpful to have an example in the placeholder of the input (for Query filter, Good Query, Total Query)

x-pack/plugins/observability/public/data/slo.ts

maryam-saeidi · 2023-01-09T15:45:34Z

x-pack/plugins/observability/public/pages/slo_edit/components/slo_edit_form.tsx

@@ -42,6 +43,7 @@ export interface Props {
 const maxWidth = 775;

 export function SloEditForm({ slo }: Props) {


In this context, SloEdit is also covering create, right? Does it make sense to reflect it somehow in the name? It was a bit hard to understand what is covered on this page in the code.

Maybe we can remove the slo_edit prefix from the components and have SloEditCreatefor the page level.

I think that's a good idea, I'll rename it.
cc @CoenWarmer Except if you strongly disagree

x-pack/plugins/observability/public/pages/slo_edit/components/slo_edit_form.tsx

maryam-saeidi · 2023-01-09T16:11:33Z

...ugins/observability/public/pages/slo_edit/components/slo_edit_form_objectives_timeslices.tsx

@@ -65,7 +67,7 @@ export function SloEditFormObjectivesTimeslices({ control }: Props) {
              min={1}
              max={120}
              step={1}
-              onChange={(event) => field.onChange(String(event.target.value))}
+              onChange={(event) => field.onChange(String(Number(event.target.value)))}


Why do you need to use Number here?

Interesting thing here. Since we are using the API response type, timeslices duration is actually a string (because it is formatted as "1m" or "1h"). When we read the response, we trim the unit part and keep only the duration, but it is still a string from the type standpoint.

Therefore, here I'm parsing the string value to a Number (so the input works well), and finally to a String because that's the expected type. If I don't Number() it, the input does not behave like this (going from value to 0 to new value) 👇🏻

Screen.Recording.2023-01-12.at.10.23.05.AM.mov

maryam-saeidi · 2023-01-09T16:17:48Z

x-pack/plugins/observability/public/pages/slo_edit/helpers/process_slo_form_values.ts

  if (!values) return undefined;

  return {
-    ...values,
+    ...omit(values, ['id', 'revision', 'createdAt', 'updatedAt', 'summary']),


Does it mean whatever data we pass to the form, will be sent to the API?

If sending this information can create an issue on the API side, I am wondering if it make sense to omit these fields there as well 🤔

kdelemme · 2023-01-12T15:38:27Z

Nice Job! 👏🏻

Tested locally, I've added some comments in the code and had some questions in general that probably are not in the scope of this ticket just noticed them while reviewing:

I didn't see a loader for the SLO list page

Create/Edit SLO

Index input is invalid at the start, even before user started filling the form

Create SLO is disabled but it is not clear why

It would be better to have it enabled but when clicked, we show invalid inputs

I saw in creating a rule in security, there is a possibility to only show one step at a time, maybe it can be useful here as well

Since we only have KQL input, can we disable clicking on the KQL part of the input?

I think it will be helpful to have an example in the placeholder of the input (for Query filter, Good Query, Total Query)

I created a ticket to track these comments: #148820

The index selector and KQL filter needs to be improved for sure, we should ideally have autocompletion of the fields from the index: #148306

kibana-ci · 2023-01-12T16:49:46Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/slo-schema`	65	67	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`observability`	558.6KB	560.7KB	+2.1KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/slo-schema`	65	67	+2

History

💚 Build #98407 succeeded 5c62cfa3a576f29fdf4687edaeafaf30f7e1cd81
💔 Build #98391 failed d94beaddbe36bb2da19c8a5ac70357bd9b5bc054
💔 Build #98345 failed 62c2ff028e1477dd6e87250e434c526e8dcb386e
💔 Build #98328 failed ee02a53aec9f9ac98a1addbf4d5919cd1debc165
💔 Build #98187 failed 920eff05170ce929223817de678da32f98820187
💔 Build #98180 failed a8a1e8183f3d23125e0c1ce259d2ab7029a50de7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @kdelemme

kdelemme self-assigned this Jan 4, 2023

kdelemme mentioned this pull request Jan 4, 2023

[SLO] [SLOEditForm] Handle partial index and empty filter for KQL indicator #148306

Closed

kdelemme marked this pull request as ready for review January 4, 2023 19:58

kdelemme requested a review from a team as a code owner January 4, 2023 19:58

kdelemme requested a review from CoenWarmer January 4, 2023 19:59

kdelemme force-pushed the feat/slo-form-create branch from a8a1e81 to 920eff0 Compare January 4, 2023 21:02

kdelemme commented Jan 5, 2023

View reviewed changes

maryam-saeidi reviewed Jan 9, 2023

View reviewed changes

kdelemme added 10 commits January 12, 2023 10:07

Handle partial index matching

6499fa8

Fix storybook

312eefc

Use CreateSLOInput type

202fd69

Handle initial timeslice window value

de4a54e

Add 90d time window and budgeting method translations

b69a5d4

Move constants

3552bb7

Fix update slo

8fc2e16

Improve update slo service

5ad274e

Update tests for create mode

d09a9c8

Handle validation for timeslice budgeting method

14fc761

kdelemme added 9 commits January 12, 2023 10:07

Improve number inputs handler

3b7ea84

Fix filtering

18d6912

Fix test: remove nested describe

cb2be4e

Fix test: move creation tests in describe block

da24ab5

Fix test: move edit tests in describe block

860876a

Fix tests: update snapshots

71f0e5f

Fix test: attempt

e845008

Fix test: next attemp

8f10b82

Skip test for now

c314f9a

kdelemme force-pushed the feat/slo-form-create branch from 5c62cfa to c314f9a Compare January 12, 2023 15:07

kdelemme requested a review from maryam-saeidi January 12, 2023 15:38

kdelemme added 2 commits January 12, 2023 10:40

fix import

c5a86e0

Handle index validation when initial loading

00d9781

maryam-saeidi approved these changes Jan 13, 2023

View reviewed changes

kdelemme merged commit 7fa8094 into elastic:main Jan 13, 2023

kdelemme deleted the feat/slo-form-create branch January 13, 2023 20:21

kibanamachine added the backport:skip This commit does not require backporting label Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(slo): slo edit form improvements #148419

feat(slo): slo edit form improvements #148419

kdelemme commented Jan 4, 2023 •

edited

Loading

elasticmachine commented Jan 4, 2023

kdelemme Jan 5, 2023

kdelemme Jan 5, 2023

kdelemme Jan 5, 2023

kdelemme Jan 5, 2023

kdelemme Jan 5, 2023

maryam-saeidi Jan 9, 2023

kdelemme Jan 12, 2023

maryam-saeidi Jan 13, 2023

kdelemme Jan 5, 2023

kdelemme Jan 5, 2023

maryam-saeidi left a comment

maryam-saeidi Jan 9, 2023

kdelemme Jan 12, 2023

maryam-saeidi Jan 9, 2023

kdelemme Jan 12, 2023

maryam-saeidi Jan 9, 2023

kdelemme commented Jan 12, 2023

kibana-ci commented Jan 12, 2023

API count

		@@ -42,6 +43,7 @@ export interface Props {
		const maxWidth = 775;

		export function SloEditForm({ slo }: Props) {

feat(slo): slo edit form improvements #148419

feat(slo): slo edit form improvements #148419

Conversation

kdelemme commented Jan 4, 2023 • edited Loading

📝 Summary

🥼 Manual testing

🎥 Recording

elasticmachine commented Jan 4, 2023

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

Choose a reason for hiding this comment

maryam-saeidi 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

kdelemme commented Jan 12, 2023

kibana-ci commented Jan 12, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

API count

History

kdelemme commented Jan 4, 2023 •

edited

Loading