Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SLOs] Fix cloning SLO by opening pre filled form #172927

Merged
merged 5 commits into from
Dec 18, 2023
Merged

[SLOs] Fix cloning SLO by opening pre filled form #172927

merged 5 commits into from
Dec 18, 2023

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Dec 8, 2023 •
edited by kibanamachine
Loading

Summary

Fixes #171939

Fix cloning SLO by opening pre filled form !!

SLO.CLone.mov

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@shahzad31
Copy link
Contributor Author

/ci

@shahzad31 shahzad31 marked this pull request as ready for review December 8, 2023 14:02
@shahzad31 shahzad31 requested a review from a team as a code owner December 8, 2023 14:02
@shahzad31 shahzad31 added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes labels Dec 8, 2023
kdelemme
kdelemme previously requested changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest we leverage sloCreateWithEncodedForm(encodedForm) to open the form with prefilled values. It would remove the need for the isClone, and we won't have to load the sloId in order to fill the form. You can see an example of this when we handle errors in the use_create_slo optimistic update: https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability/public/hooks/slo/use_create_slo.ts#L87-L89

I think we would get the same result, but in a couple of lines and less code complexity in the form

@botelastic botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Dec 8, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@shahzad31 shahzad31 requested a review from kdelemme December 11, 2023 10:51
@shahzad31 shahzad31 requested a review from mgiota December 14, 2023 11:44
mgiota
mgiota reviewed Dec 18, 2023
View reviewed changes
x-pack/plugins/observability/public/hooks/slo/use_clone_slo.ts
navigateToUrl(
basePath.prepend(
paths.observability.sloCreateWithEncodedForm(
encode({ ...slo, name: `[Copy] ${slo.name}`, id: undefined })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahzad31 Just out of curiosity in the previous implementation we were using id: uuidv4(). I guess we are calling uuidv4() when clicking on the Create SLO button, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah because we were actually creating an SLO in memory. In new implementation we are just redirecting user to the pre filled form.

mgiota
mgiota reviewed Dec 18, 2023
View reviewed changes
x-pack/plugins/observability/public/pages/slo_edit/slo_edit.tsx
@@ -33,7 +33,7 @@ export function SloEditPage() {
const { sloId } = useParams<{ sloId: string | undefined }>();
const { hasAtLeast } = useLicense();
const hasRightLicense = hasAtLeast('platinum');
const { data: slo, isInitialLoading } = useFetchSloDetails({ sloId });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahzad31 Why is this not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was un-necessarily blocking rendering of the form, causing empty page splash before form opens

mgiota
mgiota reviewed Dec 18, 2023
View reviewed changes
x-pack/plugins/observability/public/pages/slos/components/card_view/slos_card_view.tsx
))}
<EuiFlexGrid columns={columns} gutterSize="m">
{sloList
.filter((slo) => slo.summary)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahzad31 Ok I see you keep only the slos that have summary, which is ok. I am just trying to understand what is the purpose of doing so. Did you notice a bug without the filtering line you added?

Can we actually have slos that do not have summary? I had the impression that there will always be a summary, cause we are creating a temp summary document at the very beginning until the actual summary document is indexed, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah but this is to prevent the same bug we are trying to fix, where we did optimistic update to make add an SLO entry in the UI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see!

mgiota
mgiota approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahzad31 I like it! I tested it locally and it works as expected. I tried to clone an SLO from both SLO lists page (card view & list view) and SLO details page and all good.

@mgiota
Copy link
Contributor

mgiota commented Dec 18, 2023 •
edited
Loading

@shahzad31 I would like your opinion regarding my comment here. Basically when we clone a group by SLO, we end up with many SLO clones (depending on the number of SLO instances) versus one SLO clone (the instance we want to clone). Do you want me to create an issue Add "clone Instance" similar to this one? With your current implementation though, this becomes less problematic, since user can delete the group by.

@shahzad31 shahzad31 dismissed kdelemme’s stale review December 18, 2023 09:36

Everything has been addressed as per review

@shahzad31 shahzad31 enabled auto-merge (squash) December 18, 2023 09:36
@shahzad31 shahzad31 merged commit 3419469 into elastic:main Dec 18, 2023
35 of 36 checks passed
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
observability 1.1MB 1.1MB -2.4KB

History

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

@kibanamachine kibanamachine mentioned this pull request Dec 18, 2023
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 18, 2023
@shahzad31
[SLOs] Fix cloning SLO by opening pre filled form (elastic#172927)
9875641 
(cherry picked from commit 3419469)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.12

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 18, 2023
@kibanamachine @shahzad31
[8.12] [SLOs] Fix cloning SLO by opening pre filled form (#172927) (#…
292b615 
…173504)

# Backport

This will backport the following commits from `main` to `8.12`:
- [[SLOs] Fix cloning SLO by opening pre filled form
(#172927)](#172927)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT
[{"author":{"name":"Shahzad","email":"shahzad31comp@gmail.com"},"sourceCommit":{"committedDate":"2023-12-18T10:52:32Z","message":"[SLOs]
Fix cloning SLO by opening pre filled form
(#172927)","sha":"3419469e39e0bb4443267bc80fb2500629c772ba","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:prev-minor","Team:obs-ux-management","v8.13.0"],"number":172927,"url":"https://github.com/elastic/kibana/pull/172927","mergeCommit":{"message":"[SLOs]
Fix cloning SLO by opening pre filled form
(#172927)","sha":"3419469e39e0bb4443267bc80fb2500629c772ba"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.13.0","labelRegex":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/172927","number":172927,"mergeCommit":{"message":"[SLOs]
Fix cloning SLO by opening pre filled form
(#172927)","sha":"3419469e39e0bb4443267bc80fb2500629c772ba"}}]}]
BACKPORT-->

Co-authored-by: Shahzad <shahzad31comp@gmail.com>
@shahzad31 shahzad31 deleted the slo-card-clone branch December 20, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgiota mgiota mgiota approved these changes

@kdelemme kdelemme Awaiting requested review from kdelemme

Assignees
No one assigned
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.12.0 v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SLO] UI crash when cloning an SLO
7 participants
@shahzad31 @apmmachine @elasticmachine @mgiota @kibana-ci @kibanamachine @kdelemme