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

feat: PC-13893 Deprecate usage of objective's value field for Composite SLOs #295

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ditrytus
Copy link
Contributor

@ditrytus ditrytus commented Aug 23, 2024

Motivation

Currently, we have value field required for all objectives in an SLO. We currently allow to not pass that field in YAML at all but it causes it to default to 0 anyway and is returned value: 0. This is inconsistent because GET yaml is different than APPLY YAML in such case.

Composite SLOs always have exactly one objective, so as such value doesn’t matter for them at all as long as it doesn't change.

Moreover, the existence of value is documented and used in examples which is confusing to new adopters of Composite SLOs because it is required, changing it will restart budget, but it doesn’t do anything.

We want to encourage and allow not setting value field for Composite SLOs while maintaining backward compatibility with users who perhaps already explicitly set it.

Summary

value field of SLO's spec.objective is now optional.

value is removed from all examples of Composite SLOs.

Nobl9 SDK is temporarily redirected unreleased version containing unreleased changes required for this to work.

Related Changes

nobl9/nobl9-go#551
nobl9/nobl9-go#549
https://github.com/nobl9/n9/pull/15406
#312
#295

Testing

  • Describe how to check introduced code changes manually.
  • Take care of test coverage on unit, integration or end-to-end levels.

Release Notes

Usage of value field in schema for objective (part of SLO resource) becomes deprecated for objectives using composite section.

  • New Composite SLOs should not set value field.
  • New Composite SLOs will still accept value: 0 for backward compatibility with older versions of Nobl9 SDK and Nobl9 Terrafrom Provider.
  • If value was previously set to 0 for Composite SLO then it should be omitted going forward.
  • If value was previously set in Composite SLO to a number other than 0 then it can no longer be updated but still will be accepted for backward compatibility.

The usage of value for SLOs using ratio or threshold SLIs does not change.

@ditrytus ditrytus changed the title feat: PC-13893 objective's value is optional for Composite SLOs feat: PC-13893 Deprecate objective's value field for Composite SLOs Aug 23, 2024
@ditrytus ditrytus changed the title feat: PC-13893 Deprecate objective's value field for Composite SLOs feat: PC-13893 Deprecate usage of objective's value field for Composite SLOs Aug 23, 2024
@ditrytus ditrytus marked this pull request as ready for review August 23, 2024 14:48
@n9-machine-user n9-machine-user added enhancement New feature or request go Pull requests that update Go code minor New functionality with at most minor breaking changes labels Sep 10, 2024
Copy link
Contributor

@piotrkwarcinski piotrkwarcinski left a comment

Choose a reason for hiding this comment

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

I'm no terraform expert but I tested the code running it locally and its logic works as it should:

  • when value = 0 composite slo is created
  • when value!=0 there's an error
  • when value doesn't exist composite slo is created

but I noticed that when you don't pass value terraform apply shows:

(⎈|arn:aws:eks:eu-central-1:922330643383:cluster/dev-demo-4:flink)➜  comp-val-test git:(PC-13893-composite-value) ✗ terraform apply                         
nobl9_project.test: Refreshing state... [id=pkw-test-tf]
nobl9_service.test: Refreshing state... [id=pkw]
nobl9_slo.composite_slo: Refreshing state... [id=pkw-test-tf-composite]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # nobl9_slo.composite_slo will be updated in-place
  ~ resource "nobl9_slo" "composite_slo" {
        id               = "pkw-test-tf-composite"
        name             = "pkw-test-tf-composite"
        # (6 unchanged attributes hidden)

      - objective {
          - display_name      = "OK" -> null
          - name              = "tf-objective-1" -> null
          - primary           = false -> null
          - target            = 0.8 -> null
          - time_slice_target = 0 -> null
          - value             = 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005 -> null
            # (1 unchanged attribute hidden)

          - composite {
              - max_delay = "45m" -> null

              - components {
                  - objectives {
                    }
                }
            }
        }
      + objective {
          + display_name = "OK"
          + name         = "tf-objective-1"
          + target       = 0.8
          + value        = 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005
            # (1 unchanged attribute hidden)

          + composite {
              + max_delay = "45m"

              + components {}
            }
        }

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.
╷
│ Warning: SLO objective unique identifier warning
│ 
│   with nobl9_slo.composite_slo,
│   on entire-budget.test.tf line 29, in resource "nobl9_slo" "composite_slo":
│   29: resource "nobl9_slo" "composite_slo" {
│ 
│ Nobl9 is introducing an SLO objective unique identifier to support the same value for different SLIs in the same SLO. As such, Nobl9 is adding a name identifier to
│ each SLO objective. Objective names can be set now, and they'll be required once grace period ends. For more detailed information, refer to:
│ https://docs.nobl9.com/features/slo-objective-unique-identifier
╵

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: 

Notice how value is a very long string when I didn't even specified it in my terraform resource I tried to apply:

terraform {
  required_providers {
    nobl9 = {
      source  = "nobl9.com/nobl9/nobl9"
      version = "0.29.2"
    }
  }
}

provider "nobl9" {
  organization     = "nobl9-dev"
  project          = "default"
  ingest_url       = "https://dev-demo-1.nobl9.dev/api"
  okta_org_url     = "https://accounts.nobl9.dev"
  okta_auth_server = ""
  client_id        = ""
  client_secret    = ""
}

resource "nobl9_project" "test" {
  name = "pkw-test-tf"
}

resource "nobl9_service" "test" {
  name    = "pkw"
  project = "pkw-test-tf"
}

resource "nobl9_slo" "composite_slo" {
  name             = "${nobl9_project.test.name}-composite"
  service          = nobl9_service.test.name
  budgeting_method = "Occurrences"
  project          = nobl9_project.test.name

  time_window {
    unit       = "Day"
    count      = 3
    is_rolling = true
  }

  objective {
    display_name = "OK"
    name         = "tf-objective-1"
    target       = 0.8
    composite {
      max_delay = "45m"
      components {
      }
    }
  }
}

I think this value can be a little confusing to users, perhaps there is a way to avoid displaying it?

@ditrytus
Copy link
Contributor Author

I removed the sentinel altogether and now I'm just setting value to default 0. This doesn't produce any weird plans and regardless of whether the user is adding value: 0 to composite or not it doesn't recognize it as an update, so I think it works even better.

Copy link
Contributor

@piotrkwarcinski piotrkwarcinski left a comment

Choose a reason for hiding this comment

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

lgtm!

ditrytus added a commit that referenced this pull request Sep 23, 2024
## Motivation

`value` field of `objective` for Composite SLO is being
[deprecated](#295).
Before the update in can be made, a N9 platform first needs to be
[updated](nobl9/n9#15406) and new version of SDK
needs to be [released](nobl9/nobl9-go#549). To
be able to release changes in the platform, e2e tests of the latest
released SDK and Terraform Provider need to pass against it.

## Summary

E2E test assertions will pass with and without
[changes](nobl9/n9#15406) in N9 platform.

## Related changes

nobl9/nobl9-go#551
nobl9/nobl9-go#549
nobl9/n9#15406
#312
#295

## Testing

Run `make test/e2e` on both environments containing and not containing
nobl9/n9#15406.
ditrytus added a commit to nobl9/nobl9-go that referenced this pull request Sep 23, 2024
## Motivation

`value` field of `objective` for Composite SLO is being
[deprecated](#549). Before the
update in SDK can be made, a N9 platform first needs to be
[updated](nobl9/n9#15406). To be able to release
changes in the platform, e2e tests of the latest released SDK need to
pass against it.

## Summary

E2E test assertions will pass with and without
[changes](nobl9/n9#15406) in N9 platform.

## Related changes

#551
#549
nobl9/n9#15406
nobl9/terraform-provider-nobl9#312
nobl9/terraform-provider-nobl9#295

## Testing

Run `make test/e2e` on both environments containing and not containing
nobl9/n9#15406.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request go Pull requests that update Go code minor New functionality with at most minor breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants