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.

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

[SLO] Create SLO with specified ID #156425

Closed
kdelemme opened this issue May 2, 2023 · 4 comments · Fixed by #157057
Closed

[SLO] Create SLO with specified ID #156425

kdelemme opened this issue May 2, 2023 · 4 comments · Fixed by #157057
Assignees
Labels
Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.9.0

Comments

@kdelemme
Copy link
Contributor

kdelemme commented May 2, 2023

📝 Summary

When creating an SLO, we should allow an optional ID to be specified, and use this ID for the SLO.
When not specified, we should fallback to a uuidv1 as we currently do.

The optional ID should be length limited to avoid problematic usage when used in the related transform ID.

To simplify SLO management through infrastructure as code, we should allow the creation of an SLO with a custom id instead of relying on a uuidv1 generated on the server.

This change requires a few changes in the repository and the schema:

  • Saving an SLO requires first to find any existing SLO by it's attributes.id, then overwrite it (update) or save a new one
  • Find an SLO by ID requires to use the find api with attributes.id
  • Delete an SLO by ID requires to first find the SLO and then deletes it.

When creating an SLO, if an existing SLO already exists for the given id, we should return a 409 conflict.
The SLO id schema should be enforced with at least a 8-char alphanumerical string, up to 32-char.

✅ Acceptance criteria

  • If the same ID already exists, the API should reject with 400 "ID already exists"
  • On the UI, throw a nice error toast
@kdelemme kdelemme added Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.9.0 labels May 2, 2023
@elasticmachine
Copy link
Contributor

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

@kdelemme
Copy link
Contributor Author

kdelemme commented May 5, 2023

@CoenWarmer you've added this. Are we sure that's the expected behaviour we want?

If the same ID already exists, the API should reject with 400 "ID already exists"
On the UI, throw a nice error toast

Our UI does not provide a way to specify an id, It's only an API details for external (e.g. terraform) client. Therefore, we don't need to handle this case in our UI (yet, maybe never).

For the 400, not sure it's the preferred response for idempotent request. Maybe a 409, but probably more a 200/201. Basically, the responsibility of choosing a unique identifier is on the client, then the server treats the request as a no-op if it already exists, or create the slo if it does not. @wandergeek can shed some light here.

@CoenWarmer
Copy link
Contributor

@CoenWarmer you've added this. Are we sure that's the expected behaviour we want?

If the same ID already exists, the API should reject with 400 "ID already exists"
On the UI, throw a nice error toast

Our UI does not provide a way to specify an id, It's only an API details for external (e.g. terraform) client. Therefore, we don't need to handle this case in our UI (yet, maybe never).

For the 400, not sure it's the preferred response for idempotent request. Maybe a 409, but probably more a 200/201. Basically, the responsibility of choosing a unique identifier is on the client, then the server treats the request as a no-op if it already exists, or create the slo if it does not. @wandergeek can shed some light here.

Hey @kdelemme, this is what was suggested while discussing this ticket in the Inbox session while you were out. We tried to interpret the use case on the basis of the description. And we (apparently incorrectly) assumed the UI implications. Feel free to adjust the acceptance criteria how you see fit.

@wandergeek
Copy link
Contributor

wandergeek commented May 17, 2023

For the 400, not sure it's the preferred response for idempotent request. Maybe a 409, but probably more a 200/201. Basically, the responsibility of choosing a unique identifier is on the client, then the server treats the request as a no-op if it already exists, or create the slo if it does not. @wandergeek can shed some light here.

Thanks for this @kdelemme, I really appreciate it. Part of me thinks a 400/409 is preferable because if someone accidentally use the create API when they should've used the update API, they could be lead into a false sense that the request worked when it didn't.

Edit: Which looks like that's exactly what you did. Perfect!

@kdelemme kdelemme self-assigned this May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.9.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants