feat(slo): Create SLO with specified ID #157057

kdelemme · 2023-05-08T19:53:14Z

📝 Summary

This PR allows the option to provide an slo id upon creation. The slo id must be between 8 and 36 characters.
When the id is not specified, we fallback on a uuidv1.
Since the slo id is not always a uuidv1, we cannot keep using the slo id as the related saved object ID. Therefore, the slo repository has been updated.

When creating an SLO with an ID already in use, we return a 409 Conflict.

🖼️ Screenshots

Screenshots

🥼 Testing

Create an SLO with a provided ID, the response should be a 200. Then, retry and the response should become 409.

curl --request POST \
  --url http://localhost:5601/kibana/api/observability/slos \
  --header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \
  --header 'Content-Type: application/json' \
  --header 'kbn-xsrf: oui' \
  --data '{
	"id": "my-own-id",
	"name": "latency logs service c7352bb3-8bce-4777-9a24-56a7cb8e3684 1683652240",
	"description": "latency from logs",
	"indicator": {
		"type": "sli.kql.custom",
		"params": {
			"index":  "service-logs*",
			"good": "latency < 150",
			"total": "",
			"filter": "host:5a3b50ed-13ed-48db-9b10-377b856507ca",
			"timestampField": "@timestamp"
		}
	},
	"timeWindow": {
		"duration": "30d",
		"isRolling": true
	},
	"budgetingMethod": "occurrences",
	"objective": {
		"target": 0.90
	}
}'

apmmachine · 2023-05-08T19:53:30Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

elasticmachine · 2023-05-09T15:18:22Z

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

x-pack/plugins/observability/server/domain/services/validate_slo.ts

CoenWarmer · 2023-05-11T13:28:06Z

x-pack/plugins/observability/server/lib/rules/slo_burn_rate/executor.test.ts

@@ -103,7 +126,7 @@ describe('BurnRateRuleExecutor', () => {
  });

  it('throws when the slo is not found', async () => {
-    soClientMock.get.mockRejectedValue(new Error('NotFound'));
+    soClientMock.find.mockRejectedValue(new SLONotFound('SLO [inexistent] not found'));


Nit: non-existent

CoenWarmer · 2023-05-11T13:29:24Z

x-pack/plugins/observability/server/services/slo/slo_repository.test.ts

+      });
+    });
+
+    it('throws when the SLO id already exists and "throwOnConflict" is true', async () => {


CoenWarmer · 2023-05-11T13:33:08Z

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

@@ -66,36 +66,58 @@ export interface SLORepository {
 export class KibanaSavedObjectsSLORepository implements SLORepository {
  constructor(private soClient: SavedObjectsClientContract) {}

-  async save(slo: SLO): Promise<SLO> {
+  async save(slo: SLO, options = { throwOnConflict: false }): Promise<SLO> {


Shouldn't we set this to true by default? I mean, most cases in this PR pass an explicit true. I'm wondering in which case a consumer would not want it to throw on conflict?

The edit flow saves with throwOnConflict set to false. So if we change the default, we need to explicitly set the false value in the edit flow.
I can change the default value, the create service will call save without options, while the edit service will call it with throwOnConflict: false.
IMO, both ways work for me

CoenWarmer

Looks good; left some questions.

…-ref HEAD~1..HEAD --fix'

kibana-ci · 2023-05-16T13:48:12Z

💚 Build Succeeded

Buildkite Build
Commit: 4d1d0bd

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	400	404	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	480	484	+4
total			+6

History

💚 Build #127149 succeeded c64b55d49bacef90d22365f949de74689fa4b52f
💚 Build #126655 succeeded 7ae2c268578c1904128e43f195aff93075a07e3e
💔 Build #126629 failed 3ef76e43f807d74d4178fa09ea72f63179b5ee71
💔 Build #126609 failed 32c67326d66d16053930da49dcbaacb4873b3f4d
💔 Build #126581 failed 6c82e528b384be6b00f0acc7abf571e559d15093
💔 Build #126359 failed 3eeb1befc0fbc17cf0c1255a3c83e86039a310c4

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

cc @kdelemme

kdelemme added the v8.9.0 label May 8, 2023

kdelemme self-assigned this May 8, 2023

kdelemme added the Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" label May 8, 2023

kdelemme force-pushed the feat/create-slo-with-id branch from 3eeb1be to 6c82e52 Compare May 9, 2023 14:19

kdelemme marked this pull request as ready for review May 9, 2023 15:18

kdelemme requested a review from a team as a code owner May 9, 2023 15:18

kdelemme added the release_note:skip Skip the PR/issue when compiling release notes label May 9, 2023

kdelemme force-pushed the feat/create-slo-with-id branch from 32c6732 to 3c3ac8c Compare May 9, 2023 15:23

CoenWarmer reviewed May 11, 2023

View reviewed changes

x-pack/plugins/observability/server/domain/services/validate_slo.ts Show resolved Hide resolved

CoenWarmer reviewed May 11, 2023

View reviewed changes

kdelemme requested a review from CoenWarmer May 11, 2023 14:12

CoenWarmer approved these changes May 12, 2023

View reviewed changes

kdelemme and others added 7 commits May 16, 2023 08:33

Allow provided id

fbe47de

allow for conflict (edit)

4a39f76

Update repository tests

4254855

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

5b2cf6e

…-ref HEAD~1..HEAD --fix'

Update openAPI

f91a511

Update burn rate rule tests

5d680c6

use synonyme

4d1d0bd

kdelemme force-pushed the feat/create-slo-with-id branch from c64b55d to 4d1d0bd Compare May 16, 2023 12:33

kdelemme merged commit 16941fd into elastic:main May 16, 2023

kibanamachine added the backport:skip This commit does not require backporting label May 16, 2023

jasonrhodes pushed a commit that referenced this pull request May 17, 2023

feat(slo): Create SLO with specified ID (#157057)

a07f155

kdelemme deleted the feat/create-slo-with-id branch September 14, 2023 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(slo): Create SLO with specified ID #157057

feat(slo): Create SLO with specified ID #157057

kdelemme commented May 8, 2023 •

edited

Loading

apmmachine commented May 8, 2023

elasticmachine commented May 9, 2023

CoenWarmer May 11, 2023

CoenWarmer May 11, 2023

CoenWarmer May 11, 2023

kdelemme May 11, 2023

CoenWarmer left a comment

kibana-ci commented May 16, 2023

ESLint disabled line counts

Total ESLint disabled count

feat(slo): Create SLO with specified ID #157057

feat(slo): Create SLO with specified ID #157057

Conversation

kdelemme commented May 8, 2023 • edited Loading

📝 Summary

🖼️ Screenshots

🥼 Testing

apmmachine commented May 8, 2023

🤖 GitHub comments

elasticmachine commented May 9, 2023

CoenWarmer May 11, 2023

Choose a reason for hiding this comment

CoenWarmer May 11, 2023

Choose a reason for hiding this comment

CoenWarmer May 11, 2023

Choose a reason for hiding this comment

kdelemme May 11, 2023

Choose a reason for hiding this comment

CoenWarmer left a comment

Choose a reason for hiding this comment

kibana-ci commented May 16, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

kdelemme commented May 8, 2023 •

edited

Loading