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

[8.10] backport SLO fixes #168231

Merged
merged 6 commits into from
Oct 10, 2023
Merged

[8.10] backport SLO fixes #168231

merged 6 commits into from
Oct 10, 2023

Conversation

kdelemme
Copy link
Contributor

@kdelemme kdelemme commented Oct 6, 2023 •
edited by simianhacker
Loading

🍒 Summary

This PR backports the following commits into 8.10 for release scheduled on 8.10.4:

Release Notes

This PR is a mix of fixes and enhancements:

  • Improve the SLO search bar, fixing the issue with focus lost when the list was refreshing
  • Handle gracefully any errors when the preview data API was failing to execute for any reason.
  • Fix Historical SLI and error budget calculation when data was intermittent and had gaps.
  • Optimize the summary transforms by running them unattended (always retry)
  • Improve resilliency of the SLO update use case, handling any errors gracefully with proper rollback.
  • Improve error handling from the react hooks

Testing notes

  • For fix(slo): preview data no response error #167909 and fix(slo): rollback when errors happen during the update use case #168142, set up a user with a role with the following:
    • Cluster Privileges: manage_transform
    • Index Privileges:
      • .slo-*: all
      • high-*: read and read_cross_cluster
    • Kibana: All spaces All privileges
    • Create an SLO with the elastic superuser
    • When you log in as this user, try to create a new SLO and then edit (hit save) the SLO the elastic user created. For the new SLO you should get an error that says you don't have the proper permissions and then end up on the create screen (not saved). When you hit save on the SLO (that the elastic user created) you should end up with the same error without changing the original SLO.

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Documentation preview:

@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!)

@kdelemme kdelemme changed the base branch from main to 8.10 October 6, 2023 14:02
@kdelemme kdelemme marked this pull request as ready for review October 6, 2023 14:04
@simianhacker simianhacker self-requested a review October 6, 2023 14:07
@kdelemme kdelemme self-assigned this Oct 6, 2023
@kdelemme kdelemme added Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Feature:SLO labels Oct 6, 2023
@elasticmachine
Copy link
Contributor

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

@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.0MB 1.0MB +416.0B

History

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

cc @kdelemme

@mgiota mgiota self-requested a review October 6, 2023 18:31
simianhacker
simianhacker approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

LGTM... I tested it with an under privileged user, it worked as expected. I started ES without a transform node, the new error message handler caught it perfectly and returned me to the "Create SLO" form. I also created and SLO with a user who had the correct privileges, then edit the privileges to remove read access to high-* and the transform continued to run but stopped indexing data due to not being able to read the data. Once I restored the privilege, it caught back up.

@mgiota
Copy link
Contributor

mgiota commented Oct 9, 2023 •
edited
Loading

I am currently reviewing this. Code review looks good. I am testing it locally.

I started ES without a transform node

@simianhacker @kdelemme How can I do this?

@mgiota
Copy link
Contributor

mgiota commented Oct 9, 2023 •
edited
Loading

@kdelemme I created a user with read only privileges for SLO, but I don't see the 2 SLOs I have.

Screen.Recording.2023-10-09.at.13.53.08.mov

I might probably be missing something, when creating my custom slo read role. How do you create a read only SLO role? I didn't specify anything to the index privileges. Do I have to?

Screenshot 2023-10-09 at 13 58 21

@kdelemme
Copy link
Contributor Author

@mgiota Yes you are missing the following index Privileges: .slo-* => read

@mgiota
Copy link
Contributor

mgiota commented Oct 10, 2023 •
edited
Loading

@kdelemme Thanks! I added the Index Privilege and now works as expected. Corresponding actions are deactivated
Screenshot 2023-10-10 at 14 44 03 Screenshot 2023-10-10 at 14 44 17

I played a bit more with custom roles. Here's a role I created:

  • Index privilege .slo-* => all
  • Kibana privilege > Custom > SLO > ALL

When I try to edit the slo, timestamp field is not preselected and the SLI preview is empty. Not sure if a real user would create a role the way I created it, so not sure if this is a bug or not. What do you think?

Screen.Recording.2023-10-10.at.14.59.20.mov

@kdelemme
Copy link
Contributor Author

When I try to edit the slo, timestamp field is not preselected and the SLI preview is empty. Not sure if a real user would create a role the way I created it, so not sure if this is a bug or not. What do you think?

That's expected, since you did not give any privileges (at least read) on the source index (.e.g the high-cardinality admin console index).

@mgiota
Copy link
Contributor

mgiota commented Oct 10, 2023

@kdelemme I tested it and everything works as expected!

@kdelemme kdelemme merged commit afa98f7 into elastic:8.10 Oct 10, 2023
17 checks passed
@kdelemme kdelemme deleted the backport/slo/8.10.4 branch October 10, 2023 16:01
kdelemme referenced this pull request Oct 10, 2023
@kdelemme
[8.11] fix(slo): handle permission error (#167933) (#168248)
636a833 
# Backport

This will backport the following commits from `main` to `8.11`:
- [fix(slo): handle permission error
(#167933)](#167933)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Kevin
Delemme","email":"kevin.delemme@elastic.co"},"sourceCommit":{"committedDate":"2023-10-05T13:17:32Z","message":"fix(slo):
handle permission error
(#167933)","sha":"335fc9b2409855f4aeebf360c0747141b2fcf03b","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:skip","Team:
Actionable
Observability","Feature:SLO","v8.12.0"],"number":167933,"url":"https://github.com/elastic/kibana/pull/167933","mergeCommit":{"message":"fix(slo):
handle permission error
(#167933)","sha":"335fc9b2409855f4aeebf360c0747141b2fcf03b"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/167933","number":167933,"mergeCommit":{"message":"fix(slo):
handle permission error
(#167933)","sha":"335fc9b2409855f4aeebf360c0747141b2fcf03b"}},{"url":"https://github.com/elastic/kibana/pull/168231","number":168231,"branch":"8.10","state":"OPEN"}]}]
BACKPORT-->
@mistic
Copy link
Member

mistic commented Oct 10, 2023

Those changes didn't make it on time into the latest BC for 8.10.3. Updating the labels.

@mistic mistic added v8.10.4 and removed v8.10.0 labels Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simianhacker simianhacker simianhacker approved these changes

@mgiota mgiota mgiota approved these changes

Assignees

@kdelemme kdelemme

Labels
Feature:SLO release_note:fix Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.10.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kdelemme @apmmachine @elasticmachine @kibana-ci @mgiota @mistic @simianhacker