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

chore(slo): general enhancement #164723

Merged
merged 10 commits into from
Aug 28, 2023
Merged

chore(slo): general enhancement #164723

merged 10 commits into from
Aug 28, 2023

Conversation

kdelemme
Copy link
Contributor

@kdelemme kdelemme commented Aug 24, 2023 •
edited
Loading

Resolves #164724
Resolves https://github.com/elastic/actionable-observability/issues/127
Resolves #164184
Resolves #163943
Resolves #164780

Summary

This PR enhances a few things:

  • ✨ Enables some hooks only when the list of slo is not empty to avoid unnecessary calls
  • ✨ Forces stop the slo transforms to mitigate failure state
  • 🐛 Updates replica settings for summary index template to use 0-1 instead of 0-all
  • ✨ Use the instanceId for the viewInAppUrl of an SLO burn rate alert.
  • ✨ Disable SLO saved objects import/export
  • 🐛 Fix [SLO] Possible 404 when calling /internal/data_views/_fields_for_wildcard #163943
  • 🐛 Update diagnosis internal API: Explication below ⬇️

Since we use the internal es client to create the index templates, index pipelines, and indices, we do not require these cluster permissions to be set on the user’s role: manage_ilm, manage_index_templates, manage_index_pipelines anymore.
Therefore when the diagnosis API is trying to fetch the index template, pipelines, etc.. to check their existence, it fails with insufficient privileges. I want to avoid having to require these privileges, especially for “read only” user.

So I have removed this kind of check, and only use the security hasPrivileges function to check two set of permission:

  1. write privilege which requires a manage_transform cluster privilege, and .slo-* index all privilege.
  2. read privilege which requires only a .slo-* index read privilege.

Overall the new set of permission required to use the SLO features is as follow:
For a user that creates/edits/manages SLOs:

  • cluster: manage_transform privilege
  • index: .slo-* -> all (+ read and view_index_metadata on any indices we want to create SLO upon)
  • features: Observability > SLO > All (on all spaces or a specific one)

For a user that read SLOs only:

  • cluster: none
  • index: .slo-* -> read
  • features: Observability > SLO > Read (on all spaces or a specific one)

@kdelemme kdelemme self-assigned this Aug 24, 2023
@kdelemme kdelemme added backport release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.10.0 labels Aug 24, 2023
@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 marked this pull request as ready for review August 24, 2023 14:35
@kdelemme kdelemme requested a review from a team as a code owner August 24, 2023 14:35
@elasticmachine
Copy link
Contributor

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

simianhacker
simianhacker previously approved these changes Aug 24, 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 (thanks for getting the import/export change in)

x-pack/plugins/observability/server/assets/component_templates/slo_summary_settings_template.ts
@@ -11,7 +11,7 @@ export const getSLOSummarySettingsTemplate = (name: string) => ({
name,
template: {
settings: {
auto_expand_replicas: '0-all',
auto_expand_replicas: '0-1',
Copy link
Member

Choose a reason for hiding this comment

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

👍

x-pack/plugins/observability/server/saved_objects/slo.ts
@@ -57,7 +57,7 @@ export const slo: SavedObjectsType = {
},
management: {
displayName: 'SLO',
importableAndExportable: true,
importableAndExportable: false,
Copy link
Member

Choose a reason for hiding this comment

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

👍

@jloleysens
Copy link
Contributor

👋🏻 I have a drive-by request for another 1-liner fix to this great feature if possible!

#163943

@kdelemme kdelemme mentioned this pull request Aug 28, 2023
Closed
@kdelemme
Copy link
Contributor Author

👋🏻 I have a drive-by request for another 1-liner fix to this great feature if possible!

#163943

Including this fix :) Thanks

@simianhacker simianhacker self-requested a review August 28, 2023 17:36
@simianhacker simianhacker dismissed their stale review August 28, 2023 17:37

Needs a re-review

@kibana-ci
Copy link
Collaborator

💚 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 130 129 -1

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 -37.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observability 99.3KB 99.4KB +65.0B
Unknown metric groups

API count

id before after diff
@kbn/slo-schema 133 132 -1

History

  • 💔 Build #154196 failed 27dd44e8d7998957de74b8e925795b9b241b0399
  • 💔 Build #154170 failed db586e5f20eca4de748455c15a2a6a38b508309b
  • 💛 Build #154108 was flaky f53d6081903d83a21a0b95629ad0f4d88cbe0c2e
  • 💔 Build #153326 failed 2f3f45da13ba98bf540707af0299dd496279cdf3
  • 💚 Build #153296 succeeded 3db285d940156cd35deded470cc31d74e80d8d07
  • 💚 Build #153197 succeeded 304ea7a51ef24a29257546c25546ca33d3aed3c5

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

cc @kdelemme

simianhacker
simianhacker approved these changes Aug 28, 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.

I tested this with a read only role and a write only role for SLOs with the minimums outlined in this PR. LGTM!

@kdelemme kdelemme merged commit 733869e into elastic:main Aug 28, 2023
@kdelemme kdelemme deleted the slo-chores branch August 28, 2023 18:50
@kibanamachine kibanamachine mentioned this pull request Aug 28, 2023
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.10

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 28, 2023
@kdelemme
chore(slo): general enhancement (elastic#164723)
32608ad 
(cherry picked from commit 733869e)
kibanamachine added a commit that referenced this pull request Aug 28, 2023
@kibanamachine @kdelemme
[8.10] chore(slo): general enhancement (#164723) (#165032)
c47cdbf 
# Backport

This will backport the following commits from `main` to `8.10`:
- [chore(slo): general enhancement
(#164723)](#164723)

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

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

<!--BACKPORT [{"author":{"name":"Kevin
Delemme","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-28T18:50:38Z","message":"chore(slo):
general enhancement
(#164723)","sha":"733869e9e5774c4813126c80e8c00532ba8659ed","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["backport","release_note:skip","Team:
Actionable
Observability","v8.10.0","v8.11.0"],"number":164723,"url":"https://github.com/elastic/kibana/pull/164723","mergeCommit":{"message":"chore(slo):
general enhancement
(#164723)","sha":"733869e9e5774c4813126c80e8c00532ba8659ed"}},"sourceBranch":"main","suggestedTargetBranches":["8.10"],"targetPullRequestStates":[{"branch":"8.10","label":"v8.10.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/164723","number":164723,"mergeCommit":{"message":"chore(slo):
general enhancement
(#164723)","sha":"733869e9e5774c4813126c80e8c00532ba8659ed"}}]}]
BACKPORT-->

Co-authored-by: Kevin Delemme <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 29, 2023
@jloleysens
Merge branch 'main' into run-migrations-on-migrator
44333f6 
* main: (40 commits)
  Adjust migrations and elasticsearch service settings for serverless. (elastic#165050)
  [Security Solution] expandable flyout - add investigate in timeline f… (elastic#165025)
  [SecuritySolution] Hide create dashboard button from listing (elastic#164476)
  Construct HTTP log message only if needed (elastic#165057)
  [Security Solution] expandable flyout - add no data message in entities details and entities overview components (elastic#164955)
  Add functional tests for serverless security management UIs (elastic#164886)
  [api-docs] 2023-08-29 Daily api_docs build (elastic#165056)
  [Cloud Security][CIS GCP]cis gcp now use updated gcp field name + small last minute changes (elastic#164792)
  [Security Solution] Expandable flyout - update risk classification ui in entities overview (elastic#165022)
  [Security Solution] Fixes Preconfigured Connectors not working with Assistant (elastic#164900)
  [Security Solution] Coverage Overview follow-up 2 (elastic#164986)
  [DOCS] Add cross-link for other encryption key settings (elastic#165014)
  chore(slo): general enhancement (elastic#164723)
  Revert "[SOR] Allow optionally downgrading documents with a higher version model in API READ methods" (elastic#164991)
  [OAS] Add more Elasticsearch query rule examples (elastic#164386)
  [security_solution_cypress] Add support for options in EsArchiver.load (elastic#164988)
  [Event Log] Skip setting assets to hidden in serverless (elastic#164767)
  remove unneeded usages of isErrorResponse (elastic#164609)
  [Enterprise Search] Make network drive connector platinum (elastic#165007)
  [RAM] update api key to become public (elastic#164883)
  ...
@simianhacker
Copy link
Member

/oblt-deploy-serverless

@simianhacker simianhacker mentioned this pull request Sep 13, 2023
Closed
This was referenced Oct 1, 2023
Merged
Merged
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

Assignees

@kdelemme kdelemme

Labels
backport release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.10.0 v8.11.0
Projects
None yet
Milestone
No milestone
7 participants
@kdelemme @apmmachine @elasticmachine @jloleysens @kibana-ci @kibanamachine @simianhacker